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 Epoch_x 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()

    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