FactoryDAO contest - leastwood's results

Lines of code

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

Vulnerability details

Impact

The passThruGate() function acts as a proxy function to the beneficiary address by attaching Ether to the call. If an excess of Ether is provided to the call, only gate.ethCost will be sent to the beneficiary. Excess Ether will be forever be locked in the contract as there is no way to have it refunded to the caller.

Recommended Mitigation Steps

Consider enforcing that msg.value == gate.ethCost such that users can not provide an excess of Ether.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L261-L272 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L224-L231

Vulnerability details

Impact

Upon pool creation, stakers can opt to earn rewards by staking their deposit tokens. When these deposit tokens are withdrawn, reward tokens are sent to the owner of the receipt along with their deposit tokens. A tax is charged on any rewards accrued by the user and are allocated to the global beneficiary. By calling the withdrawTaxes() function, taxes can be withdrawn by iterating through a list of reward tokens. A single transfer failure will cause the entire function call to revert, preventing the global beneficiary from receiving their tokens on a given pool. As a result, pool creators can use this knowledge by adding a malicious token to their list of reward tokens. This token can do nothing except revert when the recipient is the global beneficiary. As a result, this malicious token will protect stakers and ensure that they receive rewards but prevent the global beneficiary from receiving their allotted tokens.

Recommended Mitigation Steps

Consider utilising try and catch statements in withdrawTaxes() to ensure failed token transfers do not revert and prevent the beneficiary with withdrawing taxes earned on other reward tokens. Transfer failures will instead fail silently, allowing the beneficiary to withdraw their allocated tokens.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/tree/main/contracts

Vulnerability details

Impact

There are several contracts which do not utilise OpenZeppelin's SafeERC20 library when performing token transfers. The FactoryDAO suite of smart contracts intends to support transfers by all tokens, including non-standard tokens such as USDT which does not revert upon failure or return a boolean value (returns void instead). This limits the usage of the protocol to only certain types of tokens.

Recommended Mitigation Steps

Consider making use of OpenZeppelin's safeTransfer() and safeTransferFrom() functions in all instances where transfer() and transferFrom() are used.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/tree/main/contracts

Vulnerability details

Impact

The FactoryDAO suite of contracts interact with any arbitrary ERC20 token. Because of this, there is a specific instance (and likely several others) where a fee-on-transfer token will not be correctly handled. PermissionlessBasicPoolFactory.fundPool() will pull funds from the pool creator and increment the pool tracker as the following:

pool.rewardFunding[i] += amount

As a result, if a fee-on-transfer token is used, too many tokens will be allotted to pool.rewardFunding, meaning some pool stakers will miss out on rewards if the pool is fully utilised.

Recommended Mitigation Steps

Consider taking a snapshot of the token balance before and after the transfer and treat the difference between these two amounts as the received token amount. This will be compatible with all types of tokens and avoid any issues where users are unable to withdraw their entire deposited amount.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L156-L173 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L220 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L279-L283 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L142

Vulnerability details

Impact

The PermissionlessBasicPoolFactory.sol contract allows anyone to add staking pools which users can participate in to earn reward tokens. Pools are segregated to ensure malicious pools cannot siphon tokens from honest pools. Upon the addition of a new pool, rewards are transferred from the creator according to the following equation:

return pool.rewardsWeiPerSecondPerToken[rewardIndex] * pool.maximumDepositWei * (pool.endTime - pool.startTime) / 1e18;

This calculation divides the multiplication by 1e18 to scale the multiplication back down as we are dealing with what is assumed to be two values using 18 decimals. However, this assumption is false and potentially dangerous as it does not allow pools to be created with a reward token that does not use 18 decimals. Because of this, fewer reward tokens will be pulled from the pool creator (upon creation) and stakers will earn a severely understated yield.

The following equation dictates how stakers accrue rewards:

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

As it can be seen, if a user earns 5 USDC tokens per second with a deposited amount of 100 USDC tokens over 5 seconds, they should receive 2500 USDC tokens denominated using 6 decimals. We can see that the calculation has a decimal format of 1e6 * 1e6 / 1e18 which obviously severely truncates the result.

Recommended Mitigation Steps

Instead of dividing the result in getMaximumRewards() and getRewards() by 1e18, it should instead be divided by the decimals of that particular reward token.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L314-L318 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L110 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L227

Vulnerability details

Impact

Upon pool creation, the pool configures a taxPerCapita value which is controlled by the global beneficiary. This global beneficiary account can effectively sandwich calls to addPool() by increasing the fee to 100% before the addition of a new pool and subsequently decreasing the fee after pool creation. As a result, all reward tokens allocated by the pool creator can be stolen by the global beneficiary. Alternatively, this can be used as a way to censor certain accounts from creating pools.

Recommended Mitigation Steps

Consider putting the setGlobalTax() function behind a timelock. This will ensure pool creators are unable to be sandwiched by a malicious global beneficiary. It makes sense for this to be on-chain as it allows users to be aware of any changes to sensitive parameters.