FactoryDAO contest - kenzo's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/FixedPricePassThruGate.sol#L48 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/FixedPricePassThruGate.sol#L53

Vulnerability details

The FPPT gate allows receiving more than gate.ethCost. But when forwarding the funds to the beneficiary, it is sending only gate.ethCost. There is no other way to retrieve funds from the contract and so msg.value-gate.ethCost will be locked. Note: the SpeedBumpPriceGate does send msg.value to the beneficiary and not just the price.

Impact

Excess ether can be sent to the contract, but it will be locked. Beneficiary will lose funds.

Proof of Concept

The gate receives ether and requires that it is enough:

        require(msg.value >= gate.ethCost, 'Please send more ETH');

And it then sends gate.ethCost, and not the full msg.value:

            (bool sent, bytes memory data) = gate.beneficiary.call{value: gate.ethCost}("");

Therefore, msg.value-gate.ethCost will be locked in the contract. As I mentioned above, the SpeedBump gate does send the whole msg.value.

Recommended Mitigation Steps

Send the whole msg.value to the beneficary.

Lines of code

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

Vulnerability details

Across the codebase, the protocol is usually checking that ERC20 transfers have succeeded by checking their return value. This check is missing in MerkleVesting's withdraw function.

Impact

If for some reason the ERC20 transfer is temporarily failing, the user would totally lose his allocation and funds. All the state variables would already have been updated at this stage, so he can't call withdraw again. There is no way to withdraw these locked tokens.

Proof of Concept

At the last point of withdraw, the function is sending the funds to the user, and does not check the return value - whether it has succeeded:

        IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);

Note that this is (nicely and rightfully) done after all the state variables have been updated. As the return value of the external call is not checked, if it has failed, the contract wouldn't know about it, and the function will finish "successfully".

Recommended Mitigation Steps

As done throughout the rest of the protocol, add a check that verifies that the transfer has succeeded.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L230 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L233 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L252 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L269

Vulnerability details

In PermissionlessBasicPoolFactory, the withdrawal functions only succeed if the transfer has succeeded in all tokens.

Impact

If a reward token's transfer has not succeeded for some reason, no other tokens can be withdrawn either. This does not only mean that rewards will be lost; At worst, this means a user's deposit will be locked - as the withdrawal of it is dependent on the withdrawal of the reward tokens.

Proof of Concept

The withdraw function is iterating over all tokens in a loop:

        for (uint i = 0; i < rewards.length; i++) {
            ...
            success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);
        }
        success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);
        require(success, 'Token transfer failed');

By looking at the success variable and condition, we can see that the function will succeed, and deposit sent back to the user, only if all token transfers have been successful. Therefore, one bad apple will spoil the bunch, and at worst make users funds locked.

A similar situation is happening in withdrawExcessRewards and withdrawTaxes.

Recommended Mitigation Steps

Consider moving to a system where as a fallback, the user can choose which specific rewards/deposit to withdraw. This only needs 1 additional storage slot - you can use 1 uint to remember which rewards have already been withdrawn (rewardTokens[0] is the first bit in the uint, rewardTokens[1] the second, etc').

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleDropFactory.sol#L77 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleDropFactory.sol#L107 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L121 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L204 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L89 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L144 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L198 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L230 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L233 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L252 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L269

Vulnerability details

The protocol checks that ERC20 transfers have succeeded using require(success). But some tokens (like USDT) do not return true on transfer success.

Impact

The protocol, which is supposed to be general purpose, will not work with these tokens. While it seems unlikely that somebody will decide to reward stakers with USDT, it might still be an option, which is not working at the moment.

Recommended Mitigation Steps

Consider using SafeERC20 for IERC20.

Lines of code

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

Vulnerability details

The reward calculation functions implicitly assume that the staking token has 18 decimals.

Impact

Incorrect amount of rewards given. As the FactoryDAO suite is supposed to be a general solution, this will cause incompatibility with certain tokens.

Proof of Concept

The staking pool calculates the amount of rewards in the following way:

rewardsLocal[i] = (secondsDiff * pool.rewardsWeiPerSecondPerToken[i] * receipt.amountDepositedWei) / 1e18;

Let's take an example:

• The reward token, REWARD, has 18 decimals
• The staking token, STAKE, has 9 decimals (eg. some Safemoon clone)
• The pool creator intended to give 1 REWARD token for 1 STAKE token per second.

He would have set rewardsWeiPerSecondPerToken to be 1e18 (=1 REWARD). Let's say Hagbard has deposited 1 STAKE token and after a second is withdrawing his reward. His reward will be calculated to be 1 * 1e18 * 1e9 / 1e18 = 1e9. But as we saw, the owner intended him to get 1e18 tokens, not 1e9. So wrong amount is given.

Recommended Mitigation Steps

Instead of dividing the calculation by 1e18:

• Consider querying the staking token for decimals()
• Consider taking the staking token decimals as parameter from the pool creator

Note: getMaximumRewards also uses the problematic 1e18 division.