pontifex - Users can't receive rewards in the actual `cvgCycle` due to unexpected error

[sherlock-admin]

pontifex

high

Users can't receive rewards in the actual cvgCycle due to unexpected error

Summary

Users are unable to receive rewards in the actual cvgCycle due to the _lockingPositionService.totalSupplyYsCvgHistories(cycleClaimed) returns 0 for the actual cvgCycle. This means users cannot claim rewards for the Token Distribution Event if the actual cvgCycle matchs with their last cycle of the token ownership or delegation.

Vulnerability Detail

According to the docs the YsDistributor.claimRewards function is designed to claim rewards associated with a particular token ID that represents a locking position NFT. It checks the ownership of the token is the owner or the delegatee and that the TDE is in the past or current cycle, meaning the rewards are available to be claimed. So users should be able to claim rewards for the actual cvgCycle. The share of rewards is computed by doing the ratio between the ysCvgBalance of the token and the totalSupply of ysCvg at the specified TDE.

        uint256 share = (_lockingPositionService.balanceOfYsCvgAt(tokenId, cycleClaimed) * 10 ** 20) /
            _lockingPositionService.totalSupplyYsCvgHistories(cycleClaimed);

But the _lockingPositionService.totalSupplyYsCvgHistories(cycleClaimed) returns 0 for the actual cvgCycle because last known total supply which the LockingPositionDelegate.totalSupplyYsCvgHistories mapping contains is for cvgControlTower.cvgCycle() - 1 cycle. https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L541-L563 https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L830-L836

Impact

Users can't receive rewards for the actual cvgCycle therefore never receive rewards for the Token Distribution Event if the actual cvgCycle matches with their last cycle of the token ownership or delegation.

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Rewards/YsDistributor.sol#L185

Tool used

Manual Review

Recommendation

Consider using the totalSupplyOfYsCvgAt function to receive total supply of ysCvg at the time

 184        uint256 share = (_lockingPositionService.balanceOfYsCvgAt(tokenId, cycleClaimed) * 10 ** 20) /
-185            _lockingPositionService.totalSupplyYsCvgHistories(cycleClaimed);
+185            _lockingPositionService.totalSupplyOfYsCvgAt(cycleClaimed);

[0xR3vert]

Hello,

Thanks a lot for your attention.

The Locking/YsDistributor contracts are designed to function in the manner observed, which aligns with their expected behavior.

When locking in a cycle before a TDE, a value called ysPartial is created, which is taken into account for that TDE. For the subsequent TDE, it's the ysTotal value that is considered. It's normal not to receive additional ysCvg & mgCvg when extending the time only. This serves as an incentive for the user to always lock for the maximum time.

Therefore, in conclusion, we must consider your issue as invalid.

Regards, Convergence Team

[nevillehuang]

Hi @0xR3vert @walk-on-me @shalbe-cvg,

Hi could you further clarify how your comments link to the issue highlighted? This issue seems to not be pointing to the inconsistency in computation of ysCvg and mgCvg when extending lock time.

[walk-on-me]

The recommendation given here would break the code. Indeed, totalSupplyAt gives the value in the future of the totalSupply of YsCvg.

What Main Shadow Hare is saying is that we cannot claim rewards linked to a TDE on a TDE. This is the design, we want users to be able to claim their rewards on TDE + 1.

[ChechetkinVV]

Escalate

The Documentation clearly states that users can request rewards in the current cycle: Checks that the TDE is in the past or current cycle, meaning the rewards are available to be claimed.

The check in the claimRewards function allows users to make a corresponding request.

On the one hand, there is the documentation for the code and the completely unambiguous requirement for verification, and on the other hand, comments from the sponsor that the behavior of the protocol should be different. In any case, the issue points out discrepancies in the documentation, code, and expected behavior that need to be resolved.

The fact that recommendations written on the basis of documentation cannot be implemented in the protocol cannot affect the validity of the issue as a whole.

[sherlock-admin2]

Escalate

The Documentation clearly states that users can request rewards in the current cycle: Checks that the TDE is in the past or current cycle, meaning the rewards are available to be claimed.

The check in the claimRewards function allows users to make a corresponding request.

On the one hand, there is the documentation for the code and the completely unambiguous requirement for verification, and on the other hand, comments from the sponsor that the behavior of the protocol should be different. In any case, the issue points out discrepancies in the documentation, code, and expected behavior that need to be resolved.

The fact that recommendations written on the basis of documentation cannot be implemented in the protocol cannot affect the validity of the issue as a whole.

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.

[nevillehuang]

@ChechetkinVV Documentation error is not considered valid issues unless the core functionality is impacted or there is a scenario where there could be a fund loss/permanent DoS impact. Since this is a design decision stated by the protocol and no fix will be implemented, imo this issue should remain as invalid.

[ChechetkinVV]

@ChechetkinVV Documentation error is not considered valid issues unless the core functionality is impacted or there is a scenario where there could be a fund loss/permanent DoS impact. Since this is a design decision stated by the protocol and no fix will be implemented, imo this issue should remain as invalid.

In this case, the check in the code corresponds to the documentation. The fact that the expected behavior of the protocol was different first became known from a comment from the sponsor. I can assume that other elements of the protocol are carried out in accordance with the documentation provided. Therefore, we can conclude that users would expect behavior in accordance with the documentation and would not be able to receive rewards, as written in the impact of this issue. This report, as well as the https://github.com/sherlock-audit/2023-11-convergence-judging/issues/110 report, highlighted this problem from different sides.

[nevillehuang]

@ChechetkinVV

I think the most appropriate decision is to invalidate both this issue and #110, as although there is differing logic in the documentation, it is not the intended design decision to allow claiming during a TDE cycle.

While it may be a coincidence, your issue brought up here essentially makes #110 a non-issue even if a user claim at the exact TDE cycle.

On the other hand, I can see how it is unfair to the validity thresholds for watsons, however, I also believe this is why a sufficiently lenient enough validity threshold is in place for such issues.

[Czar102]

Agree with the Lead Judge. Planning to leave the issue as is.

[Czar102]

Result: Low Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ChechetkinVV: rejected