Gauge weight reduction via GaugeController.change_gauge_weight() breaks global accounting, potentially causing both outsized reward distribution and loss of user rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L185-L206
https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L145-L182
https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L173-L183
https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L146-L161
https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L124-L132

Vulnerability details

Impact

Users may be given outsized rewards; therefore it is possible for LendingLedger's CANTO balance to drop below the reward amount of a valid user claim, in which case that claim will fail. Furthermore, gauge decay accounting is made inaccurate.

Proof of Concept

Note the below code and inline comments for GaugeController.change_gauge_weight():

    function _change_gauge_weight(address _gauge, uint256 _weight) internal {
        uint256 old_gauge_weight = _get_weight(_gauge); // current weight of the gauge
        uint256 old_sum = _get_sum(); // global weight
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;

        points_weight[_gauge][next_time].bias = _weight; // gauge's weight is set to the _weight argument. For the presented case, _weight is less than the current gauge weight
        time_weight[_gauge] = next_time; // gauge's "time checkpoint" is updated, not relevant for this issue

        uint256 new_sum = old_sum + _weight - old_gauge_weight; // new_sum is less than old_sum for this case
        points_sum[next_time].bias = new_sum; // global bias is updated
        time_sum = next_time; // global "time checkpoint" is updated, not relevant for this issue
    }

    /// @notice Allows governance to overwrite gauge weights
    /// @param _gauge Gauge address
    /// @param _weight New weight
    function change_gauge_weight(address _gauge, uint256 _weight) public onlyGovernance { // all logic is handled by the internal function _change_gauge_weight
        _change_gauge_weight(_gauge, _weight);
    }

We see here that the weight (bias) is updated for both the global state and the individual gauge, while slope is not updated at all. Consider the following: let the equation $f(t) = b - mt$ represent the gauge's decay equation before the weight is reduced by some amount $k$. (Note that $m$, or slope, is always positive in the protocol's code.) After the weight is reduced by calling change_gauge_weight(), the equation becomes $f(t) = -k + b -mt$. The slope is unchanged, but the t-axis intercept has changed from $t_{before} = b/m$ to $t_{after} = (-k + b)/m$, the latter of which is a smaller value. Now, because global "slope changes" (slope values that should be subtracted from the global slope upon their decay equations equaling zero) are stored in the changes_sum timestamp to slope mapping, which is not modified by change_gauge_weight(), there is a time period with length determined by the value of $t_1 - t_2$ during which the previous slope changes applied to the global state by user calls to vote_for_gauge_weights() remain applied even though they should have been subtracted at the start of the aforementioned time period.

This results in the global weight being less than the sum of the weights of all the individual gauges, which is an accounting error. Thus when LendingLedger.claim() calls GaugeController.gauge_relative_weight_write() on the affected time period, the calculated relative weight will be inflated, and LendingLedger.claim() will disburse outsized rewards. If the magnitude of the outsized rewards is large enough, then the LendingLedger contract may lose enough funds to where the claim() function starts to fail. Furthermore, it's also possible for the global weight to reach zero before the weight of all gauges reaches zero, possibly resulting in active gauges receiving zero rewards.

Note that this issue only applies once a gauge's weight has decayed to zero. This is guaranteed to occur at the next epoch if a gauge (having nonzero bias, nonzero slope, and t-intercept greater than the current time plus two weeks) is removed with GaugeController.remove_gauge().

Finally, reducing the weight of a gauge makes its decay equation inaccurate, as can be seen from the t-intercept changes discussed.

Tools Used

Manual Review

Recommended Mitigation Steps

This issue is difficult to fix without losing functionality in change_gauge_weights() due to the way that slope changes are calculated by the contract; the accounting logic in GaugeController may need to be changed significantly. The issue can be easily fixed by not allowing governance to reduce gauge weights.

Assessed type

Math

[141345]

slope and bias math issue
unsync accounting

[c4-sponsor]

OpenCoreCH marked the issue as sponsor confirmed

[OpenCoreCH]

Very good catch, the math in the PoC seems valid to me. I have to think about how to fix this, one way might be to calculate the new t-intercept (based on the updated bias) and update the slope changes such that they take this into consideration.

[OpenCoreCH]

Just noticed that this and #206 probably should be duplicates. While they address the problem from a different view point, both point out the same underlying problem that change_gauge_weight does not change the slope / changes_weight

[OpenCoreCH]

Fixed in mkt-market/canto-neofinance-coordinator#4

As the warden mentioned, this is not that easy to fix because one gauge may have contributed to many different changes_sum entries in a different capacity because of different user votes. I solved the issue by only allowing governance to reset weights to 0. Handling this case is a bit easier. We can reset the gauge slope to 0, subtract it from the overall slope and zero out all slope changes by iterating over the gauge-specific changes, zeroing them and subtracting them from the overall slope changes.
This is not very efficient, but it is a governance function that should only be called very rarely.

[c4-judge]

alcueca marked the issue as duplicate of #206

[c4-judge]

alcueca marked the issue as satisfactory