FactoryDAO contest - p4st13r4's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L144 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L173

Vulnerability details

Impact

Most of the FactoryDAO code use no external dependencies. When handling ERC-20 transfers, the IERC20.sol interface is imported and basic methods like transfer or transferFrom are called throughout the codebase.

However, this may lead to some issues when dealing with non ERC-20 compliant tokens such as USDT, which return void instead of boolean in transfer functions. So the following lines of code would revert if an user wanted to init a pool using USDT:

function fundPool(uint poolId) internal {
    Pool storage pool = pools[poolId];
    bool success = true;
    uint amount;
    for (uint i = 0; i < pool.rewardFunding.length; i++) {
        amount = getMaximumRewards(poolId, i);
        // transfer the tokens from pool-creator to this contract
        success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount);
        // bookkeeping to make sure pools don't share tokens
        pool.rewardFunding[i] += amount;
    }
    require(success, 'Token deposits failed');
}

This snippet is taken from PermissionlessBasicPoolFactory.sol but similar ones are used in many places around the codebase where the result of transferFrom is always checked. Everywhere the returned value is checked, except for MerkleVesting.sol: https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L173

The codebase contains many comments regarding malicious tokens etc, but in this case, the current implementation would prevent using many non-malicious, widespread tokens such as USDT.

Tools Used

Editor

Recommended Mitigation Steps

Use OZ SafeERC20 library which checks for non compliant tokens and handles them correctly. If using external libraries is out-of-scope, then write a small equivalent internal module that behaves the same as OZ’library