cergyk - Division by Zero in CvgRewards::_distributeCvgRewards leads to locked funds

[sherlock-admin]

cergyk

high

Division by Zero in CvgRewards::_distributeCvgRewards leads to locked funds

Summary

The bug occurs when CvgRewards::_setTotalWeight sets totalWeightLocked to zero, leading to a division by zero error in
CvgRewards::_distributeCvgRewards, and blocking cycle increments. The blocking results in all Cvg locked to be unlockable permanently.

Vulnerability Detail

The function _distributeCvgRewards of CvgRewards.sol is designed to calculate and distribute CVG rewards among staking contracts. It calculates the cvgDistributed for each gauge based on its weight and the total staking inflation. However, if the totalWeightLocked remains at zero (due to some gauges that are available but no user has voted for any gauge), the code attempts to divide by zero.

The DoS of _distributeCvgRewards will prevent cycle from advancing to the next state State.CONTROL_TOWER_SYNC, thus forever locking the users’ locked CVG tokens.

Impact

Loss of users’ CVG tokens due to DoS of _distributeCvgRewards blocking the state.

Code Snippet

    function _setTotalWeight() internal {
        ...
❌      totalWeightLocked += _gaugeController.get_gauge_weight_sum(_getGaugeChunk(_cursor, _endChunk)); //@audit `totalWeightLocked` can be set to 0 if no gauge has received any vote
        ...
    }

    function _distributeCvgRewards() internal {
        ...
        uint256 _totalWeight = totalWeightLocked;
        ...
        for (uint256 i; i < gaugeWeights.length; ) {
            /// @dev compute the amount of CVG to distribute in the gauge
❌          cvgDistributed = (stakingInflation * gaugeWeights[i]) / _totalWeight; //@audit will revert if `_totalWeight` is zero
        ...

/**
* @notice Unlock CVG tokens under the NFT Locking Position : Burn the NFT, Transfer back the CVG to the user.  Rewards from YsDistributor must be claimed before or they will be lost.    * @dev The locking time must be over
* @param tokenId to burn
*/
function burnPosition(uint256 tokenId) external {
...
❌      require(_cvgControlTower.cvgCycle() > lastEndCycle, "LOCKED"); //@audit funds are locked if current `cycle <= lastEndCycle`
...
    }

https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Rewards/CvgRewards.sol#L321

Tool used

Recommendation

If the _totalWeight is zero, just consider the cvg rewards to be zero for that cycle, and continue with other logic:

-cvgDistributed = (stakingInflation * gaugeWeights[i]) / _totalWeight; 
+cvgDistributed = _totalWeight == 0 ? 0 : (stakingInflation * gaugeWeights[i]) / _totalWeight;

[0xR3vert]

Hello,

Thanks a lot for your attention.

We are aware of the potential for a division by zero if there are no votes at all in one of our gauges. However, this scenario is unlikely to occur in reality because there will always be votes deployed (by us and/or others) in the gauges. Nevertheless, your point is valid, and we will address it to be prepared for this case.

Therefore, in conclusion, we must acknowledge your issue as correct, even though we are already aware of it.

Regards,
Convergence Team

[nevillehuang]

Since DoS is not permanent where in as long as protocol/users themselves vote for the gauge, I think this is a low severity issue.

[CergyK]

Escalate

Escalating based on latest comment:

Since DoS is not permanent where in as long as protocol/users themselves vote for the gauge, I think this is a low severity issue.

If we reach this case (totalWeights == 0), the DoS is permanent. There would be no other way to reset this variable, and all user funds would be locked permanently.

It is acknowledged that there is a low chance of this happening, but due to the severe impact and acknowledged validity this should be a medium

[sherlock-admin2]

Escalate

Escalating based on latest comment:

Since DoS is not permanent where in as long as protocol/users themselves vote for the gauge, I think this is a low severity issue.

If we reach this case (totalWeights == 0), the DoS is permanent. There would be no other way to reset this variable, and all user funds would be locked permanently.

It is acknowledged that there is a low chance of this happening, but due to the severe impact and acknowledged validity this should be a medium

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]

@CergyK As mentioned by the sponsor, they will always ensure there is votes present to prevent this scenario, so I can see this as an "admin error" if the scenario is allowed to happen, but I also see your point given this was not made known to watsons. If totalWeight goes to zero, it will indeed be irrecoverable.

Unlikely but possible, so can be valid based on this sherlock rule

Causes a loss of funds but requires certain external conditions or specific states.

[Czar102]

I believe this impact warrants medium severity. Planning to accept the escalation.

[Czar102]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• CergyK: accepted

[walk-on-me]

Hello dear auditor,

we fixed this issue.

You can find how on the following link :

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

[IAm0x52]

Fix looks good. Now utilizes a ternary operator to prevent division by zero