`MultiRewardStaking._accrueRewards` can lead to loss of rewards for lower decimal tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L406

Vulnerability details

Rewards accrual is computed in the following way:

406: deltaIndex = accrued.mulDiv(uint256(10**decimals()), supplyTokens, Math.Rounding.Down).safeCastTo224();

This can lead to truncation for low decimal tokens:

Consider an instance with DAI as the asset, with a total of 1e7 shares minted, and WBTC as a reward token, in a dynamic reward system (rewards.rewardsPerSecond == 0).

An admin calls fundReward with 0.09 WBTC (approximately worth $2,000 at the time of this report). This line calls _accrueRewards with amount == 0.09 WBTC == 9 * 1e6:

• deltaIndex = accrued * 10**decimals / totalSupply
• = 9 1e6 1e18 / 1e25
• = 9 * 1e24 / 1e25
• = 0

deltaIndex == 0, meaning the amount transferred is not accounted for in rewardsInfos[].index. As rewardsInfos[].index is the variable used to compute users shares of rewards in _accrueUser(), this means this amount of rewards will inevitably be stuck in the contract.

Impact

Medium

Tools Used

Manual Analysis

Mitigation

You could add a check deltaIndex > 0, but this will introduce new constraints for other functions of the protocol that accrue rewards.

A check in fundRewards that accrued * 10**decimals > totalSupply would suffice.

[c4-judge]

dmvt marked the issue as duplicate of #404

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt changed the severity to 3 (High Risk)

[c4-judge]

dmvt marked the issue as satisfactory