FactoryDAO contest - 0x1337's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L144-L146 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L194-L198 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L230-L233

Vulnerability details

Impact

The PermissionlessBasicPoolFactory contract is incompatible with deflationary tokens (defined as tokens with transfer tax where the amount of tokens received does not equal to the amount of tokens being sent). However, there is no check in the contract and anyone can use a deflationary token as either depositToken or rewardTokens for a pool. This has several negative impacts: 1) some users might be unable to withdraw the depositToken that they have deposited to the contract; 2) if a hacker observes that a deflationary token is used as the rewardToken in another pool, the hacker could add a new pool where the depositToken is the rewardToken of the other pool, and use a dummy token as rewardToken for the new pool that he/she creates. In this case, the hacker could still get the same amount of his depositToken back, but the rewardToken of the other pool will be drained from the contract due to transfer tax and depositors to the other pool might be unable to withdraw due to insufficient rewardToken balance. In both cases, some users fund might be lost / unwithdrawable.

Proof of Concept

Deflationary tokens charge a tax amount to any token transfers, and come in different flavors (e.g. using the tax to add liquidity to a Dex, or convert to native token and pay a dividend, etc.), and have become quite popular (e.g. various Safemoon forks).

The PermissionlessBasicPoolFactory contract does not check for any potential token transfer tax, and is not compatible with deflationary tokens either as depositToken or rewardToken.

As shown in the code block below from the deposit() function, the amount recorded in the receipt is the same amount as what the user is transferring to the token. If the depositToken charges a 10% transfer tax and a user deposits 100 tokens, the balance of the depositToken in the contract will only increase by 90, but the amount being recorded, which is what the user can withdraw, is set to be 100.

        receipt.amountDepositedWei = amount;
        receipt.timeDeposited = block.timestamp;
        receipt.owner = msg.sender;

        bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);

In line 233 which is in the withdraw() function, copied below, you could see that the amount a user can withdraw is based on the recorded amount in the receipt.

success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);

Using the example above, that would be 100. Thus, if two users have each deposited 100 depositToken to this contract, only one user could call the withdraw() function, because the transaction would revert due to insufficient balance (and the variable success would be false) when the second user tries to call withdraw(), resulting in loss of user fund.

The PermissionlessBasicPoolFactory also should not accept a deflationary token as a reward token in a pool. If a hacker observes that a certain deflationary token is used as the reward token in another pool (call it pool 0), the hacker could create a new pool (call it pool 1) and use the same deflationary token as the depositToken. The hacker could then calculate a deposit amount that would drain the reward token in pool 0.

As an example, let's still assume that this particular deflationary token charges a 10% transfer tax, and the current reward token balance of pool 0 is 100. The hacker could deposit 1000 token to pool 1 that he/she created (in which case the token balance of the contract will increase by 900), and immediately withdraw the entire amount from pool 1. Because the deposit receipt will record 1000, the PermissionlessBasicPoolFactory will transfer 1000 token to the hacker, of which 100 tokens should actually be the reward token of pool 0. In essense, due to the comingling of fund in the contract, a hacker could drain any reward token that charges a transfer tax with the technique above. Users who have deposited to pool 0 would be unable to withdraw their depositToken, due to insufficient reward token balance and the lack of an emergencyWithdaw() function which lets user withdraw their deposited tokens without worrying about the rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Recommend disallowing deflationary tokens to be used as either depositToken or rewardToken of any pool. This could be done by checking that the amount being sent to the contract equals the increase in the contract's token balance.