`killGauge()` will lead to wrong calculation of emission

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-09-fenix-finance/blob/main/contracts/core/VoterUpgradeableV2.sol#L239 https://github.com/code-423n4/2024-09-fenix-finance/blob/main/contracts/core/VoterUpgradeableV2.sol#L630-L639

Vulnerability details

Description

The VoterUpgradeableV2.sol contract has killGauge() that disables the gauge to prevent it from further rewards distribution, only the address with GOVERNANCE_ROLE role can call it. the killGauge() only updates three state variables

File: VoterUpgradeableV2.sol
227:     function killGauge(address gauge_) external onlyRole(_GOVERNANCE_ROLE) {
...
232:         delete gaugesState[gauge_].isAlive;
...
236:             delete gaugesState[gauge_].claimable;
...
240:         totalWeightsPerEpoch[epochTimestamp] -= weightsPerEpoch[epochTimestamp][state.pool];

The distribute() function will distribute rewards to pools managed by the VoterUpgradeableV2.sol contract and it will call the Minter contract by triggering update_period() function before distributing rewards.

The timeline looks like this

    Epoch_x                      Epoch_x+1  
    |-----------x-------------------|-x---------------------  
       call `killGauge()`             call `distribute()` 

When distribute() gets invoked in the timeline it will distribute the rewards of Epoch_x, The killed gauge has no weight in this epoch because its weight gets subtracted from totalWeightsPerEpoch[] in killGauge().

When the Minter invokes VoterUpgradeableV2.sol#notifyRewardAmount() to notify the contract of the reward amount to be distributed for Epoch_x, we can also find in the same function how the index value gets increased

File: VoterUpgradeableV2.sol
382:     function notifyRewardAmount(uint256 amount_) external {
...
387:         uint256 weightAt = totalWeightsPerEpoch[_epochTimestamp() - _WEEK]; 
388:         if (weightAt > 0) {
389:             index += (amount_ * 1e18) / weightAt;
390:         }

the index is updated as the reward amount divided by the total weights of Epoch_x we know the weight of the disabled gauge is not included in totalWeightsPerEpoch[Epoch_x]

So, back to _distribute()

File: VoterUpgradeableV2.sol
671:     function _distribute(address gauge_) internal {
...
677:             uint256 totalVotesWeight = weightsPerEpoch[currentTimestamp - _WEEK][state.pool];
678: 
679:             if (totalVotesWeight > 0) {
...
684:                     if (state.isAlive) {
685:                         gaugesState[gauge_].claimable += amount;
686:                     } else {
687:                         IERC20Upgradeable(token).safeTransfer(minter, amount);
}

Because killGauge() doesn't delete the values of weightsPerEpoch[], it will send back amount of emissions back to Minter, which actually should get distributed between the existing pools

To summarize: the index is directly related by the value of totalWeightsPerEpoch[Epoch_x], and the killGauge() is subtracted from the weightsPerEpoch of the disabled gauge. so, the index didn't include the weight of the killed gauge, but _distribute calculates its emission and sends it back to Minter.

To understand the impact (check the Proof of Concept section for the details) in case the total emissions for Epochx is 80e18 with three active gauges (with the same amount of votes), each pool will receive 26.5e18 token But in case one gauge gets killed one scenario is the 1st gauge will receive 40e18 and the other 40e18 will get transferred back to Minter, this will leave the last gauge with 0 emissions (from here the impact is related to how gauge.sol#.notifyRewardAmount() will handle this situation with is out of scope in this contest).
Another scenario is to send 40e18 to the two gauges but the disabled gauge gets revived in the next epoch and will be able to receive his 40e18 token because the `gaugesState[gauge].index` is not updated (this will loop us to the above scenario again because the 40e18 tokens do not exist in the first time )

Impact

• One or more gauges will not receive their emissions.
• Wrong calculation of gaugesState[gauge_].claimable.
• The distribution system will be broken if the killed gauge gets revived again. The impact depends on the order of the gauges array that passed to distribut() function

Proof of Concept

Let's say now is Epoch_x +1:

• we have three pools with the same vote weight (500e18) for each of them
• index = 10e18
• total emission is: amount_ = 80e18
• the totalWeightsPerEpoch of Epoch_x is: weightAt = 1500e18 scenario_01: no gauge gets disabled each gauge will receive 26.5e18 tokens as emission
  this is how we calculate it

  
  How `notifyRewardAmount()` increase the `index`
  uint256 weightAt = 1500e18
  uint256 amount_ = 80e18

index += (amount_ 1e18) / weightAt; = (80e18 1e18)/1500e18 = 5.3e16 Now, index = 10.053e18

How distribute() calcul the amount for the 3 pools uint256 delta = index - state.index; = 10.053 e18- 10e18 = 0.053e18

uint256 amount = (totalVotesWeight delta) / 1e18; = (500e18 0.053e18)/1e18 = 26.5e18

 scenario_02: one gauge gets disabled
so the  `totalWeightsPerEpoch` of Epoch_x now is : `weightAt` = 1000e18
With the current logic, two gauges each will receive `40e18` tokens as emission + `40e18` should be sent back to Minter, which is larger than the total emission which is `80e18`  

this is how we calculate it 

How notifyRewardAmount() increase the index uint256 weightAt = 1000e18 uint256 amount_ = 80e18

index += (amount_ 1e18) / weightAt; = (80e18 1e18)/1000e18 = 8e16 Now, index = 10.08e18

How distribute() calcul the amount for the 3 pools uint256 delta = index - state.index; = 10.08 e18- 10e18 = 0.08e18

uint256 amount = (totalVotesWeight delta) / 1e18; = (500e18 0.08e18)/1e18 = 40e18

## Tools Used

Manual Review

## Recommended Mitigation Steps

One fix is to delete the `weightsPerEpoch[][]` in `killGauge()`
```diff
    function killGauge(address gauge_) external onlyRole(_GOVERNANCE_ROLE) {
...

        uint256 epochTimestamp = _epochTimestamp();
        totalWeightsPerEpoch[epochTimestamp] -= weightsPerEpoch[epochTimestamp][state.pool];
+      delete  weightsPerEpoch[epochTimestamp][state.pool];
        emit GaugeKilled(gauge_);
    }

However, the fix should take into consideration how the Minter calculates the emissions for every epoch (is it a fixed value every time or depending on how many gauges are active )

Assessed type

Invalid Validation

[c4-judge]

alcueca marked the issue as primary issue

[alcueca]

Killing gauges can be considered normal operation, therefore the finding and severity are valid.

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report