code-423n4 / 2022-02-concur-findings

2 stars 0 forks source link

[WP-H1] Rewards distribution can be disrupted by a early user #193

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/02d286253cd5570d4e595527618366f77627cdaf/contracts/ConvexStakingWrapper.sol#L184-L188

Vulnerability details

https://github.com/code-423n4/2022-02-concur/blob/02d286253cd5570d4e595527618366f77627cdaf/contracts/ConvexStakingWrapper.sol#L184-L188

if (_supply > 0 && d_reward > 0) {
    reward.integral =
        reward.integral +
        uint128((d_reward * 1e20) / _supply);
}

reward.integral is uint128, if an early user deposits with just 1 Wei of lpToken, and make _supply == 1, and then transferring 5e18 of reward_token to the contract.

As a result, reward.integral can exceed type(uint128).max and overflow, causing the rewards distribution to be disrupted.

Recommendation

Consider wrap a certain amount of initial totalSupply at deployment, e.g. 1e8, and never burn it. And consider using uint256 instead of uint128 for reward.integral. Also, consdier lower 1e20 down to 1e12.

GalloDaSballo commented 2 years ago

The warden has shown a way to break the uin128 accounting system in place.

This is contingent on frontrunning the pool and depositing a small amount to cause the division to fail. Additionally, this will cause a DOS that prevents other people from depositing.

I believe that this could be unstuck by continuously (via loop) depositing 1 wei as to slowly increase the totalSupply again.

Mitigation can be attained by either refactoring or by ensuring that the first deposit is big enough (18 decimals) to keep numbers to rational values.

Because the finding is contingent on a setup and because tokens will be rescuable, I believe Medium Severity to be more appropriate