Nested Finance contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/fe6f9ef7783c3c84798c8ab5fc58085a55cebcfc/contracts/FeeSplitter.sol#L214

Vulnerability details

Impact

The latest FeeSplitter code introduced an ETH token address which is supposed to be used for token deposits judging from the getAmountDue comment:

contract FeeSplitter is Ownable, ReentrancyGuard {
  address private constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

  /// @param _token ERC20 payment token address (or ETH_ADDR)
  function getAmountDue(address _account, IERC20 _token) public view returns (uint256) {}
}

However:

1. ETH can never be released from the contract. The only ETH transfer happens when unwrapping WETH in releaseTokens with _token == WETH.
2. ETH can never be deposited to this contract as receive reverts if not coming from WETH and sendFees is not payable

Recommended Mitigation Steps

If ETH should indeed be a valid token, accept it by removing the restriction in receive or making sendFees payable. There should be a special case in releaseTokens/releaseTokensNoETH for ETH.

function releaseTokens(IERC20[] calldata _tokens) external nonReentrant {
    uint256 amount;
    for (uint256 i = 0; i < _tokens.length; i++) {
        amount = _releaseToken(_msgSender(), _tokens[i]);
        if (address(_tokens[i]) == ETH) {
            (bool success, ) = _msgSender().call{ value: amount }("");
            require(success, "FS: ETH_TRANFER_ERROR");
        } else if ...
    }
}

If ETH should not be used, remove the ETH definition again and fix the comment in getAmount.