`PermissionlessBasicPoolFactory.sol` Does Not Support Reward Tokens With Decimals Other Than 18

[code423n4]

Lines of code

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#L156-L173 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L142 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L220

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.

[illuzen]

Duplicate #47

[gititGoro]

Reducing severity as rewards represent leakage, not unsafe deposits.

[liveactionllama]

Marking this as it invalid as it appears to be a near-exact duplicate of issue #244