Let’s review this piece of code and see what vulnerabilities might be hiding in there.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
interface IERC20 {
    function approve(address spender, uint256 amount) external returns (bool);
    function transferFrom(address from, address to, uint256 amount) external returns (bool);
    function transfer(address recipient, uint256 amount) external returns (bool);
}
interface IAaveLendingPool {
    function deposit(address asset, uint256 amount, address onBehalfOf, uint16 referralCode) external;
}
contract YieldAggregator {
    address public owner;
    IERC20 public immutable usdc;
    IAaveLendingPool public aave;
    mapping(address => uint256) public deposits;
    constructor(address _usdc, address _aave) {
        owner = msg.sender;
        usdc = IERC20(_usdc);
        aave = IAaveLendingPool(_aave);
    }
    function deposit(uint256 amount) external {
        require(amount > 0, "Amount must be > 0");
        usdc.transferFrom(msg.sender, address(this), amount);
        usdc.approve(address(aave), amount);
        aave.deposit(address(usdc), amount, msg.sender, 0); 
        deposits[msg.sender] += amount;
    }
    function setAave(address _aave) external {
        require(msg.sender == owner, "Not owner");
        aave = IAaveLendingPool(_aave);
    }
    function emergencyWithdraw(address token, uint256 amount) external {
        require(msg.sender == owner, "Not owner");
        IERC20(token).transfer(owner, amount);
    }
}

Time to think

time’s up.

• We usually pay special attention to external contract calls , for checking reentrancy vulnerabilities [★★★★★★]

  function deposit(uint256 amount) external {
      require(amount > 0, "Amount must be > 0");
      usdc.transferFrom(msg.sender, address(this), amount);  // ← call externals
      usdc.approve(address(aave), amount);
      aave.deposit(address(usdc), amount, msg.sender, 0);     // ← call externals
  deposits[msg.sender] += amount;  // ← Status updates come last
  }

  Why external contract calls could be exposed to reentrancy attacks?

  EVM is single-threaded, and each call to external contracts runs synchronously. If the external contract is a bad guy, it calls the deposit function again when it is called by transferFrom, then we are getting back to the deposit function again, that’s causing transferFrom to run again, it’s transferring again and again and again… till it cleans up all of your money.

  So, this is the fix:

  function deposit(uint256 amount) external nonReentrant {
      require(amount > 0, "Amount must be > 0");
      deposits[msg.sender] += amount;  // ← Update the state first
      usdc.transferFrom(msg.sender, address(this), amount);
      usdc.approve(address(aave), amount);
      aave.deposit(address(usdc), amount, msg.sender, 0);
  }

  Either add a nonReentrant modifier or swap these two line, we do both. (nonReentrant modifier requires _status != _ENTERED , to prevent reentrancy )

• Single point of failure (SPOF) [★★★★★★]

  function setAave(address _aave) external {
      require(msg.sender == owner, "Not owner");
      aave = IAaveLendingPool(_aave);  // ← this can change address by owner alone
  }

  ​ The owner alone can change some critical variables ，better not to allow this.

  ​ Advice: consider using multi-sig

  
  

• backdoor [★★★★★★]

  function emergencyWithdraw(address token, uint256 amount) external {
      require(msg.sender == owner, "Not owner");
      IERC20(token).transfer(owner, amount);  // ← rug pull
  }

  ​

  1. It’s called an ‘emergency withdrawal’ , but it’s really a rug pull backdoor.

  2. Users fully trust the owner, but they have no protection

     Fix: Might add this line: require(token != address(usdc), "Cannot withdraw USDC"); to protect USDCs. Anyway, the fix always depends on the situation.

• Compatibility [★★☆☆☆☆]

  • usdc.transferFrom(msg.sender, address(this), amount);
    // ← The return value of `transferFrom` isn't handled

    Some ERC-20 tokens don’t return a bool from transferFrom, and they won’t revert even if the transfer fails, for example the early USDT didn’t return a bool from transfer or transferFrom , recommend using SafeERC20 instead.

    using SafeERC20 for IERC20;
    usdc.safeTransferFrom(msg.sender, address(this), amount);

• Design defect [★★☆☆☆☆]

  You’re only depositing money, and there are no withdrawals for users. I think this it is a design flaw.

  We need to be sure to provide a solution to withdraw funds that got stock somewhere , that convinces users to trust the contract.

文章目录