aslanbek - Adding or changing a pool without `massUpdatePools` will result in either too many or too little rewards than intended

[sherlock-admin4]

aslanbek

medium

Adding or changing a pool without massUpdatePools will result in either too many or too little rewards than intended

Summary

Functions set and add allow the owner to change existing pool's configuration, or add a new one. They both have bool _withUpdate parameter, which if true calls massUpdatePools before changing the config. The issue is that every time set or add are called with _withUpdate = false, all pools will have incorrect rewards between their last update and current timestamp, as the change to allocPoints and totalAllocPoints would apply retrospectively.

Vulnerability Detail

Proof of Concept

There's 2 empty pools: A and B, allocPointA = allocPointB = 50; _pointsPerBlock = 30e18;

1. Alice stakes worth of 100 USD in pool A
2. Bob stakes worth of 100 USD in pool B
3. 1000 blocks later owner adds pool C with allocPoint = 50, _withUpdate = false
4. Alice frontruns owner's txn with updatePool(A): pointRewardA = _pointsPerBlock * blocksPassed * allocPointA / totalAlloc = 30e18 * 1000 * 50 / 100 = 15_000e18
5. Owner's txn is mined

Now, updatePool(B) will result in: pointRewardB = _pointsPerBlock * blocksPassed * allocPointB / totalAlloc = 30e18 * 1000 * 50 / 150 = 10_000e18. So all stakers of pool B will receive 1/3 less rewards than all stakers of A, despite their pools having the same weight.

On the other hand, If owner used _withUpdate = true, both Alice and Bob would have received 1/2 of rewards for the first 1000 blocks, as they should.

Similarly, if a pool is set to a higher allocPoint without massUpdatePools, all stakers of that pool will receive more rewards than they should, and the contract in aggregate may temporarily emit more rewards than _pointsPerBlock (if other pools were updated later than the changed one).

Impact

Too little / too many rewards for stakers whose pools were not updated right before functions set or add were called.

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L427-L428

Tool used

Manual Review

Recommendation

Make massUpdatePools mandatory for SophonFarming#add and SophonFarming#set, instead of optional.

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

invalid because it would require the admin to make a mistake by inputting a false boolean

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/sophon-org/farming-contracts/commit/6d117397d57a39a865cee0621cabf9154fb6fd71

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.