sherlock-admin3 commented 5 months ago

Audinarey

high

`user.rewardSettled` after withdrawal and `_pendingPoints(...)` calculation breaks accounting for user 's pending points

Summary

user.rewardDebt is the amount of point entitled to an already distributed to a user and recorded. user.rewardSettled on the other hand is the amount of reward entitled to a used and pending distribution. However, the _pendingPoints(...) calculation breaks accounting for the protocol due to its discrepancy in the actual reward point earned at the point of withdrawal leading to a potential leak of value as described below

File: SophonFarming.sol
...
597:         user.rewardSettled = userAmount * pool.accPointsPerShare / 1e18 + user.rewardSettled - user.rewardDebt;

...
619:         user.rewardDebt = userAmount *
621:             pool.accPointsPerShare /
622:             1e18;
623: 

699:     function withdraw(uint256 _pid, uint256 _withdrawAmount) external {
700:         if (isWithdrawPeriodEnded()) {
701:             revert WithdrawNotAllowed();
702:         }
703:         if (_withdrawAmount == 0) {
704:             revert WithdrawIsZero();
705:         }
...
719:         user.rewardSettled = userAmount * pool.accPointsPerShare / 1e18 + user.rewardSettled - user.rewardDebt;
720: 
....
737:         user.rewardDebt = userAmount *
738:             pool.accPointsPerShare /
739:             1e18;

365:     function _pendingPoints(uint256 _pid, address _user) internal view returns (uint256) {
366:         PoolInfo storage pool = poolInfo[_pid];
367:         UserInfo storage user = userInfo[_pid][_user];
368: 
369:         uint256 accPointsPerShare = pool.accPointsPerShare * 1e18; // *1e36
370: 
371:         uint256 lpSupply = pool.amount;
372:         if (getBlockNumber() > pool.lastRewardBlock && lpSupply != 0) {
373:             uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());
374: 
375:             uint256 pointReward = blockMultiplier * pointsPerBlock * pool.allocPoint / totalAllocPoint;
376: 
377:             accPointsPerShare = pointReward * 1e18 / lpSupply + accPointsPerShare;
378:         }
379: 
380:         // We do some fancy math here. Basically, any point in time, the amount of points
381:         // entitled to a user but is pending to be distributed is:
382:         //   pending reward = (user.amount * pool.accPointsPerShare) - user.rewardDebt
383:         return user.amount * accPointsPerShare / 1e36 +  user.rewardSettled - user.rewardDebt;
384:     }

Vulnerability Detail

For simplicity, I'll use ETH as the deposit asset Assume that: pool.amount = 0 pool.accPointsPerShare = 100e18 points user.rewardDebt = 0

at t = 0s, user deposits 100ETH and his rewards as calculated
- pool.amount = 100ETH
- on L597 user.rewardSettled = 10000e18 and
- on L621 user.rewardDebt = 10000e18
- undistributed rewards = 10000e18
at t 1000s user withdraws 90ETH remaining 10ETh and his rewards as calculated
- pool.amount = 10ETH
- on L723 user.rewardSettled = 10000e18 and
- on L737 user.rewardDebt = 1000e18 and
- undistributed rewards = 9000e18
_pendingPoints(...) is called for user
- pool.amount has reduced to 10ETH,
- pointReward has not changed because blockMultiplier, pointsPerBlock pool.allocPoint, totalAllocPoint have not changed at this point
- accPointsPerShare will increase ( pointReward * 1e18 / 10e18 + 100e18) as shown on L377 to say 120e18
pending points is calculated as shown on L383
```
user.amount * accPointsPerShare / 1e36 +  user.rewardSettled - user.rewardDebt;
// 10e18 * 120e18 * 1e18 / 1e36 + 10000e18
// = 11200e18
```
As shown below, the the discrepancy between the user.rewardSettled and the pending points returned by _pendingPoints(...).

11200e18 - 10000e18  = 1200e18

The user is seen here to have a pending 1200e18 rewards manufactured out of thin air

Impact

This:

breaks core protocol functionality because of discrepancies
possible leak of value in points distribution as user can earn more points than thy are entitled to

Code Snippet

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L574-L595

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L719-L725

https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol#L379-L384

Tool used

Manual Review

Recommendation

A trivial solution is not in sight at this time, however returning the user.rewardSettled could just be the solution here whenever _pendingPoints(...) is called and the blockMultiplier, pointsPerBlock pool.allocPoint, totalAllocPoint have not changed. Also worhty of note is that decreasing the pool's lpSupply actually increases the accPointsPerShare.

sherlock-admin4 commented 5 months ago

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

0xmystery commented:

invalid because blockMultiplier, pointsPerBlock pool.allocPoint, totalAllocPoint are universally linked to pointReward

Audinarey commented 5 months ago

Escalate

I am not disputing that the aforementioned variables are not universally linked. Notice that I mentioned in my explanation that

pointReward has not changed because blockMultiplier, pointsPerBlock pool.allocPoint, totalAllocPoint have not changed at this point

However, whenever _pendingPoints(...) is called, the accPointsPerShare will not only be fetched directly from the storage but will now depend on lpSupply as shown on L377 and considering that blockMultiplier, pointsPerBlock, pool.allocPoint, totalAllocPoint have not changed (in this scenario), pointReward remains the same but lpSupply has reduced, as such the calculated _pendingPoints(...) for the user will differ from the user.rewardSettled as shown on L383


365:     function _pendingPoints(uint256 _pid, address _user) internal view returns (uint256) {
366:         PoolInfo storage pool = poolInfo[_pid];
367:         UserInfo storage user = userInfo[_pid][_user];
368: 
369:         uint256 accPointsPerShare = pool.accPointsPerShare * 1e18; // *1e36
370: 
371:         uint256 lpSupply = pool.amount;
372:         if (getBlockNumber() > pool.lastRewardBlock && lpSupply != 0) {
373:             uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());
374: 
375:             uint256 pointReward = blockMultiplier * pointsPerBlock * pool.allocPoint / totalAllocPoint;
376: 
377:   @>        accPointsPerShare = pointReward * 1e18 / lpSupply + accPointsPerShare;
378:         }
379: 
380:         // We do some fancy math here. Basically, any point in time, the amount of points
381:         // entitled to a user but is pending to be distributed is:
382:         //   pending reward = (user.amount * pool.accPointsPerShare) - user.rewardDebt
383: @>      return user.amount * accPointsPerShare / 1e36 +  user.rewardSettled - user.rewardDebt;

Hence the calculation,

user.amount * accPointsPerShare / 1e36 +  user.rewardSettled - user.rewardDebt;
// 10e18 * 120e18 * 1e18 / 1e36 + 10000e18
// = 11200e18

And thus, the the discrepancy between the user.rewardSettled and the pending points returned by _pendingPoints(...).

_pendingPoints(uint256 _pid, address _user) - user.rewardSettled
11200e18 - 10000e18  = 1200e18

The _pendingPoints(...) shows that the user has 1200e18 pints more than he actually does

sherlock-admin3 commented 5 months ago

Escalate

I am not disputing that the aforementioned variables are not universally linked. Notice that I mentioned in my explanation that

pointReward has not changed because blockMultiplier, pointsPerBlock pool.allocPoint, totalAllocPoint have not changed at this point

However, whenever _pendingPoints(...) is called, the accPointsPerShare will not only be fetched directly from the storage but will now depend on lpSupply as shown on L377 and considering that blockMultiplier, pointsPerBlock, pool.allocPoint, totalAllocPoint have not changed (in this scenario), pointReward remains the same but lpSupply has reduced, as such the calculated _pendingPoints(...) for the user will differ from the user.rewardSettled as shown on L383
365:     function _pendingPoints(uint256 _pid, address _user) internal view returns (uint256) {
366:         PoolInfo storage pool = poolInfo[_pid];
367:         UserInfo storage user = userInfo[_pid][_user];
368: 
369:         uint256 accPointsPerShare = pool.accPointsPerShare * 1e18; // *1e36
370: 
371:         uint256 lpSupply = pool.amount;
372:         if (getBlockNumber() > pool.lastRewardBlock && lpSupply != 0) {
373:             uint256 blockMultiplier = _getBlockMultiplier(pool.lastRewardBlock, getBlockNumber());
374: 
375:             uint256 pointReward = blockMultiplier * pointsPerBlock * pool.allocPoint / totalAllocPoint;
376: 
377:   @>        accPointsPerShare = pointReward * 1e18 / lpSupply + accPointsPerShare;
378:         }
379: 
380:         // We do some fancy math here. Basically, any point in time, the amount of points
381:         // entitled to a user but is pending to be distributed is:
382:         //   pending reward = (user.amount * pool.accPointsPerShare) - user.rewardDebt
383: @>      return user.amount * accPointsPerShare / 1e36 +  user.rewardSettled - user.rewardDebt;
Hence the calculation,
user.amount * accPointsPerShare / 1e36 +  user.rewardSettled - user.rewardDebt;
// 10e18 * 120e18 * 1e18 / 1e36 + 10000e18
// = 11200e18
And thus, the the discrepancy between the user.rewardSettled and the pending points returned by _pendingPoints(...).
_pendingPoints(uint256 _pid, address _user) - user.rewardSettled
11200e18 - 10000e18  = 1200e18
The _pendingPoints(...) shows that the user has 1200e18 pints more than he actually does

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

mystery0x commented 5 months ago

The following assumption is wrong:

pool.amount = 0
pool.accPointsPerShare = 100e18 points
user.rewardDebt = 0

When pool.amount == 0, lpSupply will be 0 too. Hence, pool.accPointsPerShare remains as 0.

WangSecurity commented 5 months ago

I agree with the previous comment by the lead judge. Leaving some time for the escalating Watson to provide counterarguments and a fixed scenario.

If nothing is provided, planning to reject the escalation and leave the issue as it is.

WangSecurity commented 5 months ago

Result: Invalid Unique

sherlock-admin4 commented 5 months ago

Escalations have been resolved successfully!

Escalation status:

Audinarey: rejected

sherlock-audit / 2024-05-sophon-judging

Audinarey - `user.rewardSettled` after withdrawal and `_pendingPoints(...)` calculation breaks accounting for user 's pending points #163

`user.rewardSettled` after withdrawal and `_pendingPoints(...)` calculation breaks accounting for user 's pending points

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-05-sophon-judging

Audinarey - `user.rewardSettled` after withdrawal and `_pendingPoints(...)` calculation breaks accounting for user 's pending points #163

user.rewardSettled after withdrawal and _pendingPoints(...) calculation breaks accounting for user 's pending points

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

`user.rewardSettled` after withdrawal and `_pendingPoints(...)` calculation breaks accounting for user 's pending points