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.

[Czar102]

I agree that this is a borderline low/med.
I don't see a reason to discriminate nonzero deposits mod 100. That said, I am siding with the escalation – the loss here is insufficient to consider it a material loss of funds (at no point in the lifetime of the codebase will it surpass $1), and a loss of protocol functionality isn't serious enough if simply sending some dust to the contract will resolve the issue.

Planning to accept the escalation and consider this a low severity issue.

[CergyK]

@Czar102 please consider report #132 which I submitted which allows to steal an arbitrary amount from the rewards under some conditions, which is a higher impact.

My issue shares the same root cause as this one, so I did not escalate for deduplication. However if you think that this issue should be low, maybe it would be more fair to make my issue unique since the impact is sufficient.

[nevillehuang]

#132 and this #190 shares the same impact, if this is invalid, #132 should be invalid as well. Namely the following two impact:

1. Last user withdrawals can revert
2. Some users will gain more rewards at the expense of others.

Both examples present used involve relatively low amounts, so I'm unsure what is the exact impact

Comparing this issue attack path

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

and #132

-> Alice locks some small amount for lockDuration = 64 so that it increases totalSupply by exactly 1
-> Alice proceeds to lock X times using the values:

Comparing this issue 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

and #132

Under specific circumstances (if the attacker is the only one to have allocated to YS during a TDE), an attacker is able to claim arbitrarily more rewards than is due to him, stealing rewards from other participants in the protocol

My opinion is both issues should remain valid medium severity issue based on impact highlighted in both issues.

[CergyK]

After some discussion with @nevillehuang, agree that issues should stay duplicated and valid high/medium given following reasons:

• Highest impact is: loss of arbitrary amount of present/future rewards (see cergyk - A user which is the only $CVG locker for YS during a TDE, can steal rewards from other TDEs #132 for explanation)
• Necessary condition of very low YS allocation is unlikely but not impossible since YS is not central in Convergence (YS allocation could be empty and the system would be working as expected)

[deadrosesxyz]

To summarize:

• Almost certainly in regular conditions, there will be no issue for any user whatsover.
• In some rare case, (see original escalation) there could be a small overdistribution of rewards (matter of a few wei). In the rarer case all users claim their rewards, the last unstaker will be unable to do so due to lack of funds. This is even more unlikely considering any time a user claims rewards, the amount they claim is rounded down, (due to built-in round down in solidity) leading to making the overdistribution smaller/inexistent. But even if all the conditions are met, the issue can be fixed by simply sending a few wei to the contract.
• The highest impact described in cergyk - A user which is the only $CVG locker for YS during a TDE, can steal rewards from other TDEs #132 requires for the total balance to not simply be very low but in fact to be just a few wei. Not only it has to be a few wei, but it has to be a few wei for at least 12 weeks (until TDE payout). It is absolutely unrealistic to have a few wei total balance for 12 weeks.

Issue should remain Low severity

[10xhash]

I agree that the fix of sending minor amounts of all reward tokens won't cost the team any considerable loss financially. But apart from the fix, the impact under reasonable conditions of user not being able to withdraw their rewards is certainly a major one.
If issues existing on the contract are judged based on the ease of fixing/preventing, I think a lot more issues would exist under this category. Wouldn't it make almost all functionality breaks in up-gradable contracts low severity due to the fix being an upgrade?

[Czar102]

Due to the additional impact noted (thank you @CergyK) I think the loss can be sufficient to warrant a medium severity for this issue (loss of funds, but improbable assumptions are made).

[Czar102]

Result:
Medium
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• deadrosesxyz: accepted

[walk-on-me]

This issue has been solved here :

https://github.com/Cvg-Finance/sherlock-cvg/pull/4

Follow the comments :

• https://github.com/Cvg-Finance/sherlock-cvg/pull/4#discussion_r1457453181
• https://github.com/Cvg-Finance/sherlock-cvg/pull/4#discussion_r1457453459
• https://github.com/Cvg-Finance/sherlock-cvg/pull/4#discussion_r1457453906
• https://github.com/Cvg-Finance/sherlock-cvg/pull/4#discussion_r1457455387

[IAm0x52]

Fix looks good. Order of operations has been updated to consistently reflect the proper value