ZanyBonzy - Lowering the gauge weight can disrupt accounting, potentially leading to both excessive fund distribution and a loss of funds.

[sherlock-admin2]

ZanyBonzy

medium

Lowering the gauge weight can disrupt accounting, potentially leading to both excessive fund distribution and a loss of funds.

Summary

Similar issues were found by users 0xDetermination and bart1e in the Canto veRWA audit, which uses a similar gauge controller type.

Vulnerability Detail

• When the _change_gauge_weight function is called, the points_weight[addr][next_time].bias andtime_weight[addr] are updated - the slope is not.

def _change_gauge_weight(addr: address, weight: uint256):
    # Change gauge weight
    # Only needed when testing in reality
    gauge_type: int128 = self.gauge_types_[addr] - 1
    old_gauge_weight: uint256 = self._get_weight(addr)
    type_weight: uint256 = self._get_type_weight(gauge_type)
    old_sum: uint256 = self._get_sum(gauge_type)
    _total_weight: uint256 = self._get_total()
    next_time: uint256 = (block.timestamp + WEEK) / WEEK * WEEK

    self.points_weight[addr][next_time].bias = weight
    self.time_weight[addr] = next_time

    new_sum: uint256 = old_sum + weight - old_gauge_weight
    self.points_sum[gauge_type][next_time].bias = new_sum
    self.time_sum[gauge_type] = next_time

    _total_weight = _total_weight + new_sum * type_weight - old_sum * type_weight
    self.points_total[next_time] = _total_weight
    self.time_total = next_time

    log NewGaugeWeight(addr, block.timestamp, weight, _total_weight)

• The equation f(t) = c - mx represents the gauge's decay equation before the weight is reduced. In this equation, m is the slope. After the weight is reduced by an amount k using the change_gauge_weight function, the equation becomes f(t) = c - k - mx The slope m remains unchanged, but the t-axis intercept changes from t₁ = c/m to t₂ = (c-k)/m.
• Slope adjustments that should be applied to the global slope when decay reaches 0 are stored in the changes_sum hashmap. And is not affected by changes in gauge weight. Consequently, there's a time window t₁ - t₂ during which the earlier slope changes applied to the global state when user called vote_for_gauge_weights function remains applied even though they should have been subtracted. This in turn creates a situation in which the global weightis less than the sum of the individual gauge weights, resulting in an accounting error.
• So, in the CvgRewards contract when the writeStakingRewards function invokes the _checkpoint, which subsequently triggers the gauge_relative_weight_writes function for the relevant time period, the calculated relative weight becomes inflated, leading to an increase in the distributed rewards. If all available rewards are distributed before the entire array is processed, the remaining users will receive no rewards."
• The issue mainly arises when a gauge's weight has completely diminished to zero. This is certain to happen if a gauge with a non-zero bias, non-zero slope, and a t-intercept exceeding the current time is killed using kill_gauge function.
• Additionally, decreasing a gauge's weight introduces inaccuracies in its decay equation, as is evident in the t-intercept.

Impact

The way rewards are calculated is broken, leading to an uneven distribution of rewards, with some users receiving too much and others receiving nothing.

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/GaugeController.vy#L568C1-L590C1

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

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Rewards/CvgRewards.sol#L235C1-L235C91

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/GaugeController.vy#L493

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/GaugeController.vy#L456C1-L475C17

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/GaugeController.vy#L603C1-L611C54

Tool used

Manual Review

Recommendation

Disable weight reduction, or only allow reset to 0.

[0xR3vert]

Hello,

Thanks a lot for your attention.

Thank you for your insightful observation. Upon thorough examination, we acknowledge that such an occurrence could indeed jeopardize the protocol. We are currently exploring multiple solutions to address this issue.
We are considering removing the function change_gauge_weight entirely and not distributing CVG inflation on killed gauges, similar to how Curve Protocol handles their gauges.

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

Regards,
Convergence Team

[CergyK]

Escalate

The severity of this issue is low for two reasons:

• Admin endpoint, the admin can be trusted to not use this feature lightly, and take preventative measures to ensure that accounting is not disrupted, such as ensuring that there is no current votes for a gauge and locking voting to set a weight.

• _change_gauge_weight has this exact implementation in the battle-tested curve dao contract (in usage for more than 3 years now without notable issue):
  https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/GaugeController.vy

[sherlock-admin2]

Escalate

The severity of this issue is low for two reasons:

• Admin endpoint, the admin can be trusted to not use this feature lightly, and take preventative measures to ensure that accounting is not disrupted, such as ensuring that there is no current votes for a gauge and locking voting to set a weight.

• _change_gauge_weight has this exact implementation in the battle-tested curve dao contract (in usage for more than 3 years now without notable issue):
  https://github.com/curvefi/curve-dao-contracts/blob/master/contracts/GaugeController.vy

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.

[0xDetermination]

Addressing the escalation points:

1. If there are no current votes for a gauge, the weight can't be lowered. Also, locking voting is not really relevant for this issue. I don't think there are any preventative measures that can be taken other than never lowering the gauge weight.
2. This function is in the live Curve contract, but it has never been called (see https://etherscan.io/advanced-filter?fadd=0x2f50d538606fa9edd2b11e2446beb18c9d5846bb&tadd=0x2f50d538606fa9edd2b11e2446beb18c9d5846bb&mtd=0xd4d2646e%7eChange_gauge_weight)

[nevillehuang]

I think all issues regarding killing and changing weight for gauges (#18, #94, #122,#192), all the arguments are assuming the following:

1. The admin would perform appropriate measures before executing admin gated functions - To me, this is not clear cut like the way admin input would be. From sponsor confirmation, you would understand that they did not understand the severity of performing such issues so it wouldn't be unreasonable to say they are not aware enough to perform such preventive measures before this functions are called. If not, I think this should have been explicitly mentioned in known design considerations in the contest details and/or the contract details here, where the purpose of locking and pausing votes are stated.

2. Afaik, when @CergyK mentions the admin can take preventive measures such as ensuring no current vote and locking votes, then what would happen during a scenario when the current gauge that an admin wants to change weight or kill gauge (e.g. malicious token) has votes assigned. Wouldn't that essentially mean admin can never do so, and therefore breaks core functionality?

3. The admin would never call change_gauge_weight because it has never been called in a live curve contract - This is pure speculation, just because curve doesn't call it, it does not mean convergence would never call it.

[Czar102]

See my comment here.

Planning to reject the escalation.

[nevillehuang]

@0xDetermination @deadrosesxyz @10xhash

Do you think this is a duplicate of #192 because they seem similar. I am unsure if fixing one issue will fix another, given at the point of contest, it is intended to invoke both functions.

[0xDetermination]

@nevillehuang I think any issue with a root cause of 'lowering gauge weight' should be considered the same if I understand the duplication rules correctly. So it seems like these are all dupes.

[Czar102]

As long as an issue is valid and the root cause is the same, they are currently considered duplicates. Both #192 and #94 have a root cause of not handling the slope in _change_gauge_weight, despite having different impacts.

[Czar102]

Result:
High
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• CergyK: rejected

[walk-on-me]

Hello, we fixed this issue on this PR.

You can see on these comments, description of the fix :

• https://github.com/Cvg-Finance/sherlock-cvg/pull/4#discussion_r1457713428
• https://github.com/Cvg-Finance/sherlock-cvg/pull/4#discussion_r1457714042

[IAm0x52]

Fix looks good. _change_gauge_weight has been removed completely