PermissionlessBasicPoolFactory does not support fee on transfer token

[code423n4]

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

[illuzen]

Duplicate #34

[gititGoro]

Changing severity due pool isolation, rewards tokens represent leakage not lost funds and because transfer of deposited funds would fail on deposit.