bughuntoor - Certain functions should not be usable when GaugeController is locked.

[sherlock-admin2]

bughuntoor

medium

Certain functions should not be usable when GaugeController is locked.

Summary

Possible unfair over/under distribution of rewards

Vulnerability Detail

When writeStakingRewards is invoked for the first time it calls _checkpoints which sets the lock in the GaugeController to true. What this does is it doesn't allow for any new vote changes. The idea behind it is that until the rewards are fully distributed there are no changes in the gauges' weights so the distribution of rewards is correct.
However, there are multiple unrestricted functions which can alter the outcome of the rewards and result in not only unfair distribution, but also to many overdistributed or underdistributed rewards.

    function _setTotalWeight() internal {
        ICvgControlTower _cvgControlTower = cvgControlTower;
        IGaugeController _gaugeController = _cvgControlTower.gaugeController();
        uint128 _cursor = cursor;
        uint128 _totalGaugeNumber = uint128(gauges.length);

        /// @dev compute the theoric end of the chunk
        uint128 _maxEnd = _cursor + cvgRewardsConfig.maxLoopSetTotalWeight;
        /// @dev compute the real end of the chunk regarding the length of staking contracts
        uint128 _endChunk = _maxEnd < _totalGaugeNumber ? _maxEnd : _totalGaugeNumber;

        /// @dev if last chunk of the total weighted locked processs
        if (_endChunk == _totalGaugeNumber) {
            /// @dev reset the cursor to 0 for _distributeRewards
            cursor = 0;
            /// @dev set the step as DISTRIBUTE for reward distribution
            state = State.DISTRIBUTE;
        } else {
            /// @dev setup the cursor at the index start for the next chunk
            cursor = _endChunk;
        }

        totalWeightLocked += _gaugeController.get_gauge_weight_sum(_getGaugeChunk(_cursor, _endChunk));

        /// @dev emit the event only at the last chunk
        if (_endChunk == _totalGaugeNumber) {
            emit SetTotalWeight(_cvgControlTower.cvgCycle(), totalWeightLocked);
        }
    }

If any of change_gauge_weight change_type_weight or is called after the totalWeightLocked is calculated, it will result in incorrect distribution of rewards. When _distributeCvgRewards is called, some gauges may not have the same value that has been used to calculate the totalWeightLocked and this may result in distribution too many or too little rewards. It also gives an unfair advantage/disadvantage to the different gauges.

    function _distributeCvgRewards() internal {
        ICvgControlTower _cvgControlTower = cvgControlTower;
        IGaugeController gaugeController = _cvgControlTower.gaugeController();

        uint256 _cvgCycle = _cvgControlTower.cvgCycle();

        /// @dev number of gauge in GaugeController
        uint128 _totalGaugeNumber = uint128(gauges.length);
        uint128 _cursor = cursor;

        uint256 _totalWeight = totalWeightLocked;
        /// @dev cursor of the end of the actual chunk
        uint128 cursorEnd = _cursor + cvgRewardsConfig.maxChunkDistribute;

        /// @dev if the new cursor is higher than the number of gauge, cursor become the number of gauge
        if (cursorEnd > _totalGaugeNumber) {
            cursorEnd = _totalGaugeNumber;
        }

        /// @dev reset the cursor if the distribution has been done
        if (cursorEnd == _totalGaugeNumber) {
            cursor = 0;

            /// @dev reset the total weight of the gauge
            totalWeightLocked = 0;

            /// @dev update the states to the control_tower sync
            state = State.CONTROL_TOWER_SYNC;
        }
        /// @dev update the global cursor in order to be taken into account on next chunk
        else {
            cursor = cursorEnd;
        }

        uint256 stakingInflation = stakingInflationAtCycle(_cvgCycle);
        uint256 cvgDistributed;
        InflationInfo[] memory inflationInfos = new InflationInfo[](cursorEnd - _cursor);
        address[] memory addresses = _getGaugeChunk(_cursor, cursorEnd);
        /// @dev fetch weight of gauge relative to the cursor
        uint256[] memory gaugeWeights = gaugeController.get_gauge_weights(addresses);
        for (uint256 i; i < gaugeWeights.length; ) {
            /// @dev compute the amount of CVG to distribute in the gauge
            cvgDistributed = (stakingInflation * gaugeWeights[i]) / _totalWeight;

            /// @dev Write the amount of CVG to distribute in the staking contract
            ICvgAssetStaking(addresses[i]).processStakersRewards(cvgDistributed);

            inflationInfos[i] = InflationInfo({
                gauge: addresses[i],
                cvgDistributed: cvgDistributed,
                gaugeWeight: gaugeWeights[i]
            });

            unchecked {
                ++i;
            }
        }

        emit EventChunkWriteStakingRewards(_cvgCycle, _totalWeight, inflationInfos);
    }

Impact

Unfair distribution of rewards. Over/underdistributing rewards.

Code Snippet

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

Tool used

Manual Review

Recommendation

Add a lock to change_gauge_weight change_type_weight

[0xR3vert]

Hello,

Thanks a lot for your attention.

Absolutely, if we kill a gauge or change a type weight during the distribution, it would distribute wrong amounts, even though we're not planning to do that. We can make sure it doesn't happen by doing what you said: locking those functions to avoid any problems.

Therefore, in conclusion, we must consider your issue as a valid.

Regards,
Convergence Team

[nevillehuang]

I will keep this as medium as although on first look this could be "admin error", as sponsor mentioned, honest users claiming during killing of a gauge or weight change can result in inaccurate result distribution.

[CergyK]

Escalate

This is an admin function, the admin can be trusted to not call these operations when voting is routinely locked to not interfere with rewards distribution

Moreover, outside of routine cycle increments, it would be a good procedure to lock votes before the admin changes weights, as described in other reported issues such as #122

[sherlock-admin2]

Escalate

This is an admin function, the admin can be trusted to not call these operations when voting is routinely locked to not interfere with rewards distribution

Moreover, outside of routine cycle increments, it would be a good procedure to lock votes before the admin changes weights, as described in other reported issues such as #122

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]

See comments here

[Czar102]

I believe the escalation makes a valid point. Planning to consider this issue a low severity one.

[Czar102]

Result:
Low
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• CergyK: accepted

[IAm0x52]

Fix looks good. _setTotalWeight has now been folded inside _checkpoints to avoid this issue