hash - Division difference can result in a revert when claiming treasury yield and excess rewards to some users

[sherlock-admin2]

hash

medium

Division difference can result in a revert when claiming treasury yield and excess rewards to some users

Summary

Different ordering of calculations are used to compute ysTotal in different situations. This causes the totalShares tracked to be less than the claimable amount of shares

Vulnerability Detail

ysTotal is calculated differently when adding to totalSuppliesTracking and when computing balanceOfYsCvgAt.
When adding to totalSuppliesTracking, the calculation of ysTotal is as follows:

        uint256 cvgLockAmount = (amount * ysPercentage) / MAX_PERCENTAGE;
        uint256 ysTotal = (lockDuration * cvgLockAmount) / MAX_LOCK;

In balanceOfYsCvgAt, ysTotal is calculated as follows

        uint256 ysTotal = (((endCycle - startCycle) * amount * ysPercentage) / MAX_PERCENTAGE) / MAX_LOCK;

This difference allows the balanceOfYsCvgAt to be greater than what is added to totalSuppliesTracking

POC

  startCycle 357
  endCycle 420
  lockDuration 63
  amount 2
  ysPercentage 80

Calculation in totalSuppliesTracking gives:

        uint256 cvgLockAmount = (2 * 80) / 100; == 1
        uint256 ysTotal = (63 * 1) / 96; == 0

Calculation in balanceOfYsCvgAt gives:

        uint256 ysTotal = ((63 * 2 * 80) / 100) / 96; == 10080 / 100 / 96 == 1

Example Scenario

Alice, Bob and Jake locks cvg for 1 TDE and obtains rounded up balanceOfYsCvgAt. A user who is aware of this issue can exploit this issue further by using increaseLockAmount with small amount values by which the total difference difference b/w the user's calculated balanceOfYsCvgAt and the accounted amount in totalSuppliesTracking can be increased. Bob and Jake claims the reward at the end of reward cycle. When Alice attempts to claim rewards, it reverts since there is not enough reward to be sent.

Impact

This breaks the shares accounting of the treasury rewards. Some user's will get more than the actual intended rewards while the last withdrawals will result in a revert

Code Snippet

totalSuppliesTracking calculation

In mintPosition
https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L261-L263

In increaseLockAmount
https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L339-L345

In increaseLockTimeAndAmount
https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L465-L470

_ysCvgCheckpoint
https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L577-L584

balanceOfYsCvgAt calculation
https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L673-L675

Tool used

Manual Review

Recommendation

Perform the same calculation in both places

+++                     uint256 _ysTotal = (_extension.endCycle - _extension.cycleId)* ((_extension.cvgLocked * _lockingPosition.ysPercentage) / MAX_PERCENTAGE) / MAX_LOCK;
---     uint256 ysTotal = (((endCycle - startCycle) * amount * ysPercentage) / MAX_PERCENTAGE) / MAX_LOCK;

[walk-on-me]

Hello

Indeed this is a real problem due the way that the invariant :
Sum of all balanceOfYsCvg > totalSupply

And so some positions will become not claimable on the YsDistributor.

We'll correct this by computing the same way the ysTotal & ysPartial on the balanceYs & ysCheckpoint

Very nice finding, it'd break the claim for the last users to claim.

[deadrosesxyz]

Escalate
The amounts are scaled up by 1e18. The rounding down problem comes when dividing by MAX_PERCENTAGE which equals 100. Worst case scenario (which will only happen if a user deposits an amount which is not divisible by 100), there will be rounding down of up to 99 wei. Not only it is insignificant, it is unlikely to happen as it requires a deposit of an amount not divisible by 1e2.
Believe issue should be marked as low.

[sherlock-admin2]

Escalate
The amounts are scaled up by 1e18. The rounding down problem comes when dividing by MAX_PERCENTAGE which equals 100. Worst case scenario (which will only happen if a user deposits an amount which is not divisible by 100), there will be rounding down of up to 99 wei. Not only it is insignificant, it is unlikely to happen as it requires a deposit of an amount not divisible by 1e2.
Believe issue should be marked as low.

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.

[10xhash]

1. The rounding down is significant because it disallows the last claimers of a TDE cycle from obtaining their reward.
2. An attacker can perform the attack which requires no expectations from the other users.
3. The reasoning to classify non 1e2 amounts as unlikely would be the neatness part and the UI interacting users. There is a functionality provided by the CvgUtilities contract itself to lock Cvg using swap and bond tokens which shouldn't be discriminating non 1e2 amounts.

[deadrosesxyz]

Worst case scenario, only the last user will be unable to claim their rewards (even though I described above why it is highly unlikely). In the rare situation it happens, it can be fixed by simply sending a few wei to the contract.

[nevillehuang]

Imo, just off point 1 alone, this warrants medium severity at the least. The fact that a donation is required to fix this means there is a bug, and is not intended functionality of the function.