FactoryDAO contest - throttle's results

Lines of code

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

Vulnerability details

Hardcode 1e18 for all rewards token can cause addPool issue with rewards/reposit token like WBTC (8 decimals) or USDC (6 decimals). The underlying function getMaximumRewards will return 0 or smaller rewards than expected due to precision lost.

Impact

Normally this would not be a problem because maximumDepositWei or rewardsWeiPerSecondPerToken will be token with 18 decimals large enough to handle hardcode 1e18 division.

But if user want user to deposit WBTC and rewards both USDC and promoted token (either with 18 decimals or not), then this issue will occur. (Yield reward per second for USDC is really high but received rewards is 0)

Unexpected contract behaviour + Hypothetical scam = Medium severity.

Proof Of Concept

1. Add pool with MyToken(18 decimals) and USDC(8 decimals) as token rewards for depositing WBTC.
2. Max deposit WBTC should be 10 WBTC (1e9) over the period of 1 month (2592000 ~= 2e6). Rewards 100K USDC (1e12) rewardsWeiPerSecondPerToken = 1e12 / 2e6 ~= 5e5.
3. During fundPool call. MyToken have no problem, but getMaximumRewards for USDC will be calculated as 5e5 * 1e9 * 2e6 / 1e18 = 1e21 / 1e18 = 1000 = 0.001 USDC.
4. Only 0.001 USDC is transfered as reward for the pool. Plus, lots of MyToken that might be worth something.

The scam use case for this pretty much inflated price for meme/DAO token. Since no USDC is rewarded, the scammer only have to mint their token to give out. Little expense, free exposure. No user lost their WBTC.

Migitation

The easiest way is replacing hardcode / 1e18 with depositToken.decimals(). Note: still not handle meme token case like there is only 1 token with only 1e18 in circulation.

This is up to user to handle.

Lines of code

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

Vulnerability details

Impact

Fee on transfer can DoS the withdrawals

Proof of Concept

Pools creation is permissionless. Anyone can propose a pool with any ERC20 token. However, some tokens don't strictly follow ERC20 standard. For example, some tokens implement fee on transfer. In such case, the withdrawal could be DOSed

deposit() function will account for more than the actual transferred. https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L180-L202

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L230 transfer() in withdraw() function might revert if there is no enough funds. Thus users will not be able to retrieve invested funds. And pool owners might not be able to withdraw (potentially huge amounts) excess rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Consider accounting for fee-on-transfer during deposit and withdrawal

[N-01] Consider using Ownable / AccessControl

Description

In few places there are addresses with roles assigned. It's better to reuse battle-tested code. It also improves readability

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

Recommended Mitigation Steps

Consider using OZ library

-----------------------------------------------------------------