Pools and trees may be underfunded for fee-on-transfer tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L137-L149 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L80-L91 https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleDropFactory.sol#L68-L79

Vulnerability details

Pools, vesting trees, and airdrop trees may all be created with fee-on-transfer tokens. When each of these entities is funded by a transfer in, their internal accounting assumes they receive the full amount transferred. However, they may actually receive fewer tokens than the amount transferred:

PermissionlessBasicPoolFactory.sol#L137-L149

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

MerkleVesting.sol#L80-L91

    function depositTokens(uint treeIndex, uint value) public {
        // storage since we are editing
        MerkleTree storage merkleTree = merkleTrees[treeIndex];

        // bookkeeping to make sure trees don't share tokens
        merkleTree.tokenBalance += value;

        // transfer tokens, if this is a malicious token, then this whole tree is malicious
        // but it does not effect the other trees
        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
        emit TokensDeposited(treeIndex, merkleTree.tokenAddress, value);
    }

MerkleDropFactory.sol#L68-L79

    function depositTokens(uint treeIndex, uint value) public {
        // storage since we are editing
        MerkleTree storage merkleTree = merkleTrees[treeIndex];

        // bookkeeping to make sure trees don't share tokens
        merkleTree.tokenBalance += value;

        // transfer tokens, if this is a malicious token, then this whole tree is malicious
        // but it does not effect the other trees
        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
        emit TokensDeposited(treeIndex, merkleTree.tokenAddress, value);
    }

As a result, some users may be unable to claim allocated tokens due to an insufficient balance.

Recommendation: Consider comparing the before and after balance to calculate the actual amount transferred.

[illuzen]

Duplicate #34