FactoryDAO contest - wuwe1's results

Lines of code

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

Vulnerability details

Proof of Concept

A simple POC: https://gist.github.com/wuwe1/9eb5bf9e4b3f31c8db52f4fa7fac5b13

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

    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);

transferFrom will revert when pool.rewardTokens do not return boolean value as expected

Recommended Mitigation Steps

Use safeTransferFrom

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L29

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L180-L202

Vulnerability details

Proof of Concept

Since the pool.depositToken can be any token, it is possible that pool will be created with tokens that support fee on transfer. If a fee on transfer token is chosen, other user's funds might be drained. The actual amount of tokens the contract holds could be less than receipt.amountDepositedWei

1. Assume transfer fee to be 5% and PermissionlessBasicPoolFactory.sol has 200 token.
2. Alice deposit 100 token. Now, PermissionlessBasicPoolFactory.sol has 295 token.
3. Alice withdraw 100 token.
4. PermissionlessBasicPoolFactory.sol ends up having 195 token.

Alice can drain token hold by PermissionlessBasicPoolFactory.sol by doing this multiple times.

Recommended Mitigation Steps

change to

    function deposit(uint poolId, uint amount) external {
        Pool storage pool = pools[poolId];
        require(pool.id == poolId, 'Uninitialized pool');
        require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');
        require(block.timestamp < pool.endTime, 'Cannot deposit after pool ends');
        require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');
        if (pool.totalDepositsWei + amount > pool.maximumDepositWei) {
            amount = pool.maximumDepositWei - pool.totalDepositsWei;
        }
        pool.totalDepositsWei += amount;
        pool.numReceipts++;

        Receipt storage receipt = pool.receipts[pool.numReceipts];
        receipt.id = pool.numReceipts;
        // receipt.amountDepositedWei = amount;
        receipt.timeDeposited = block.timestamp;
        receipt.owner = msg.sender;

        uint256 balanceBefore = IERC20(pool.depositToken).balanceOf(address(this));
        bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
        recepit.amountDepositedWei = IERC20(pool.depositToken).balanceOf(address(this)) - balanceBefore;
        require(success, 'Token transfer failed');

        emit DepositOccurred(poolId, pool.numReceipts, msg.sender);
    }

Since every receipt can only be withdrawn once, this is safe from reentrance call to withdraw