ApprovalHelper, TransferHelper, and CallOptionalReturn all wrong

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/ApprovalHelper.sol#L7-L29 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/CallOptionalReturn.sol#L7-L38 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/TransferHub/TransferHelper.sol#L7-L51

Vulnerability details

Impact

• The CallOptionalReturn.sol provides the _callOptionalReturn() which is used to call a token contract, and return a boolean or revert based of the scenario. However it is done wrong.

  (
            bool success,
            bytes memory returndata
        ) = token.call(
            data
        );

        bool results = returndata.length == 0 || abi.decode(
            returndata,
            (bool)
        );

        if (success == false) {
            revert();
        }

        call = success
            && results
            && token.code.length > 0;

The above code is the content of the function however,

1. if the erc20 token returns false instead of reverting from a call, this function does not revert. This is gotten as the bool results but is not checked here or where ever it is called.

• ApprovalHelper is used to perform safeApprove() call on tokens, however it misses something important.

  function _safeApprove(
      address _token,
      address _spender,
      uint256 _value
  )
      internal
  {
      _callOptionalReturn(
          _token,
          abi.encodeWithSelector(
              IERC20.approve.selector,
              _spender,
              _value
          )
      );
  }

  It does not check the bool being returned from the _callOptionalReturn

• TransferHelper is used to perform _safeTransfer() and _safeTransferFrom call on tokens, however it misses something important also. It does not check the bool being returned from the _callOptionalReturn

    function _safeTransfer(
        address _token,
        address _to,
        uint256 _value
    )
        internal
    {
        _callOptionalReturn(
            _token,
            abi.encodeWithSelector(
                IERC20.transfer.selector,
                _to,
                _value
            )
        );
    }

Proof of Concept

From the EIP20 at https://eips.ethereum.org/EIPS/eip-20, it is stated that: Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned! However, this is not handled here.

Below is an ERC20 compliant token which tests the transfer functionality

import {Test, console} from "forge-std/Test.sol";

interface IERC20 {

    function transferFrom(address from, address to, uint256 amount) external returns (bool);
        function approve(address spender, uint256 amount) external returns (bool);
            function allowance(address owner, address spender) external view returns (uint256);
                function transfer(address to, uint256 amount) external returns (bool);
                    function balanceOf(address account) external view returns (uint256);
                        function totalSupply() external view returns (uint256);
}

error AllowanceBelowZero();
error ApproveWithZeroAddress();
error BurnExceedsBalance();
error BurnFromZeroAddress();
error InsufficientAllowance();
error MintToZeroAddress();
error TransferAmountExceedsBalance();
error TransferZeroAddress();

contract SimpleERC20 is IERC20 {

    string internal _name;
    string internal _symbol;

    uint8 internal _decimals;
    uint256 internal _totalSupply;

    mapping(address => uint256) internal _balances;
    mapping(address => mapping(address => uint256)) internal _allowances;

    // Miscellaneous constants
    uint256 internal constant UINT256_MAX = type(uint256).max;
    address internal constant ZERO_ADDRESS = address(0);

    event Transfer(
        address indexed from,
        address indexed to,
        uint256 value
    );

    event Approval(
        address indexed owner,
        address indexed spender,
        uint256 value
    );

    function name()
        external
        view
        returns (string memory)
    {
        return _name;
    }

    function symbol()
        external
        view
        returns (string memory)
    {
        return _symbol;
    }

    function decimals()
        external
        view
        returns (uint8)
    {
        return _decimals;
    }

    function totalSupply()
        public
        view
        returns (uint256)
    {
        return _totalSupply;
    }

    function balanceOf(
        address _account
    )
        public
        view
        returns (uint256)
    {
        return _balances[_account];
    }

    function _transfer(
        address _from,
        address _to,
        uint256 _amount
    )
        internal
        returns (bool)
    {
        if (_from == ZERO_ADDRESS || _to == ZERO_ADDRESS) {
            revert TransferZeroAddress();
        }

        uint256 fromBalance = _balances[_from];

        if (fromBalance < _amount) {
            return false;
        }

        unchecked {
            _balances[_from] = fromBalance - _amount;
            _balances[_to] += _amount;
        }

        emit Transfer(
            _from,
            _to,
            _amount
        );
        return true;
    }

    function _mint(
        address _account,
        uint256 _amount
    )
        internal
    {
        if (_account == ZERO_ADDRESS) {
            revert MintToZeroAddress();
        }

        _totalSupply += _amount;

        unchecked {
            _balances[_account] += _amount;
        }

        emit Transfer(
            ZERO_ADDRESS,
            _account,
            _amount
        );
    }

    function _burn(
        address _account,
        uint256 _amount
    )
        internal
    {
        if (_account == ZERO_ADDRESS) {
            revert BurnFromZeroAddress();
        }

        uint256 accountBalance = _balances[
            _account
        ];

        if (accountBalance < _amount) {
            revert BurnExceedsBalance();
        }

        unchecked {
            _balances[_account] = accountBalance - _amount;
            _totalSupply -= _amount;
        }

        emit Transfer(
            _account,
            ZERO_ADDRESS,
            _amount
        );
    }

    function transfer(
        address _to,
        uint256 _amount
    )
        external
        returns (bool)
    {
     return  _transfer(
            _msgSender(),
            _to,
            _amount
        );

    }

    function allowance(
        address _owner,
        address _spender
    )
        public
        view
        returns (uint256)
    {
        return _allowances[_owner][_spender];
    }

    function approve(
        address _spender,
        uint256 _amount
    )
        external
        returns (bool)
    {
     return _approve(
            _msgSender(),
            _spender,
            _amount
        );

    }

    function transferFrom(
        address _from,
        address _to,
        uint256 _amount
    )
        external
        returns (bool)
    {
        _spendAllowance(
            _from,
            _msgSender(),
            _amount
        );

       return _transfer(
            _from,
            _to,
            _amount
        );

    }

    function _approve(
        address _owner,
        address _spender,
        uint256 _amount
    )
        internal
        returns (bool)
    {
        if (_owner == ZERO_ADDRESS || _spender == ZERO_ADDRESS) {
            revert ApproveWithZeroAddress();
        }

        _allowances[_owner][_spender] = _amount;

        emit Approval(
            _owner,
            _spender,
            _amount
        );
        return true;
    }

    function _spendAllowance(
        address _owner,
        address _spender,
        uint256 _amount
    )
        internal
        returns (bool)
    {
        uint256 currentAllowance = allowance(
            _owner,
            _spender
        );

        if (currentAllowance != UINT256_MAX) {

            if (currentAllowance < _amount) {
                return false;
            }

            unchecked {
                 _approve(
                    _owner,
                    _spender,
                    currentAllowance - _amount
                );
            }

        }
        return true;
    }

    function _msgSender()
        internal
        view
        returns (address)
    {
        return msg.sender;
    }
}

contract CallOptionalReturn {

    /**
     * @dev Helper function to do low-level call
     */
    function _callOptionalReturn(
        address token,
        bytes memory data
    )
        internal
        returns (bool call)
    {
        (
            bool success,
            bytes memory returndata
        ) = token.call(
            data
        );

        bool results = returndata.length == 0 || abi.decode(
            returndata,
            (bool)
        );

        if (success == false) {
            revert();
        }

        call = success
            && results
            && token.code.length > 0;
    }
}

contract TransferHelper is CallOptionalReturn {

    /**
     * @dev
     * Allows to execute safe transfer for a token
     */

     //@audit it does not require the output from the transfer
    function _safeTransfer(
        address _token,
        address _to,
        uint256 _value
    )
        public
    {
        _callOptionalReturn(
            _token,
            abi.encodeWithSelector(
                IERC20.transfer.selector,
                _to,
                _value
            )
        );
    }

        function _safeTransferFrom(
        address _token,
        address _from,
        address _to,
        uint256 _value
    )
        public
    {
        _callOptionalReturn(
            _token,
            abi.encodeWithSelector(
                IERC20.transferFrom.selector,
                _from,
                _to,
                _value
            )
        );
    }
}

contract CounterTest is Test {
    IERC20 token;
    TransferHelper helper;

    address user1;
    function setUp() public {
      helper = new TransferHelper();
  token = new SimpleERC20();
  user1 = address(0xaaaaa);
    }

    function testTransfer() public{
      //@audit no tokens have been minted 
      vm.startPrank(user1);
      token.transfer(address(0x01), 19000000);
      helper._safeTransfer(address(token), address(0x02), 2e20);
      helper._safeTransferFrom(address(token), address(0xababab),address(0x03), 2e20);
    }

}

The foundry output is as follows:

josephdara@Josephs-Air newPOC % forge test
[⠒] Compiling...
[⠊] Compiling 1 files with 0.8.24
[⠢] Solc 0.8.24 finished in 835.48ms
Compiler run successful!

Ran 1 test for test/Counter.t.sol:CounterTest
[PASS] testTransfer() (gas: 30247)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.05ms (148.54µs CPU time)

As we can see, it passes with no failure

Tools Used

Manual Review, Foundry, Remix, ENSReport

Recommended Mitigation Steps

Use OpenZeppelin's Safe ERC20 or add require statements to accurately check return value from callOptionalReturn

Assessed type

ERC20

[c4-pre-sort]

GalloDaSballo marked the issue as primary issue

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[GalloDaSballo]

The sponsor disputed the finding in some other dup, that said it does look valid for some tokens (which may not be the ones listed in scope)

[trust1995]

The implementation is only behaving incorrectly when the token returns False, in this case is should revert. When it does not return a valid, it reverts, or it returns True, there is no impact. The scope section defines: ERC20 in scope: WETH, WBTC, LINK, DAI, WstETH, sDAI, USDC, USDT, WISE and may others in the future. (Also corresponding Aave tokens if existing) None of the specified ERC20s in the list return False on failure. The question becomes whether the may others in the future part of the scope description includes tokens with that behavior (which is fully compliant to the ERC20 spec). It seems that the current implementation will brick the ability of the sponsor to insert other tokens in the future, at least that is a plausible scenario which achieves Med impact.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked issue #245 as primary and marked this issue as a duplicate of 245