FactoryDAO contest - hubble's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L215-L234

Vulnerability details

In PermissionlessBasicPoolFactory , and multiple reward tokens configured, if there is any issues with any one rewards token, then withdraw will fail for even the principal deposit. The transferFrom of rewardTokens can fail for any known/unknown reasons, like the rewardToken contract is paused or frozen at that time, or one of the rewardToken is a malicious token, etc.,

Since this is a permissionless setup, there is a possibility of deploying pools with wrong rewardTokens, and difficult for users to discover until withdraw is called.

Impact

Loss of user funds. The deposit amount of users will get stuck in the contract and not possible to withdraw.

Proof of Concept

Contract : PermissionlessBasicPoolFactory.sol Function : withdraw()

Recommended Mitigation Steps

Need a backup function to withdraw only deposit amount. Suggested changes as given below.

In struct Receipt , instead of timeWithdrawn use two different members

   uint timeWithdrawnRewards;  // the time the rewards was withdrawn, or 0 if not withdrawn yet
   uint timeWithdrawnDeposit;  // the time the deposit was withdrawn, or 0 if not withdrawn yet

Add another function for withdrawing deposit amount.

    function withdrawDeposit(uint poolId, uint receiptId) external {
        Pool storage pool = pools[poolId];
        require(pool.id == poolId, 'Uninitialized pool');
        Receipt storage receipt = pool.receipts[receiptId];
        require(receipt.id == receiptId, 'Can only withdraw real receipts');
        require(receipt.owner == msg.sender || block.timestamp > pool.endTime, 'Can only withdraw your own deposit');
        require(receipt.timeWithdrawnDeposit == 0, 'Can only withdraw once per receipt');

        // close re-entry gate
        receipt.timeWithdrawnDeposit = block.timestamp;

        bool success = true;

        success = IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);
        require(success, 'Token transfer failed');

        emit WithdrawalOccurred(poolId, receiptId, receipt.owner);     // change the event log as appropriate
    }

Also modify the current withdraw function to take into account the modified Receipt members timeWithdrawnRewards & timeWithdrawnDeposit

Alternately , have different functions to withdrawRewards and withdrawDeposit for simplicity