sherlock-audit / 2024-06-velocimeter-judging

11 stars 7 forks source link

Minato7namikazi - `activeGaugeNumber` could be incremented multiple times for the same gauge within an epoch. #550

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

Minato7namikazi

High

activeGaugeNumber could be incremented multiple times for the same gauge within an epoch.

Vulnerability Detail

in the distribute function:

function distribute(address _gauge) public lock {
    IMinter(minter).update_period();
    _updateFor(_gauge); // should set claimable to 0 if killed
    uint _claimable = claimable[_gauge];
    if (_claimable > IGauge(_gauge).left(base) && _claimable / DURATION > 0) {
        claimable[_gauge] = 0;
        if((_claimable * 1e18) / currentEpochRewardAmount > minShareForActiveGauge) {
            activeGaugeNumber += 1;
        }

        IGauge(_gauge).notifyRewardAmount(base, _claimable);
        emit DistributeReward(msg.sender, _gauge, _claimable);
    }
}

The bug is in the condition for incrementing the activeGaugeNumber:

if((_claimable * 1e18) / currentEpochRewardAmount > minShareForActiveGauge) {
    activeGaugeNumber += 1;
}

This condition is checked every time distribute is called for a gauge, which can lead to the activeGaugeNumber being incremented multiple times for the same gauge within an epoch. This could result in an inflated activeGaugeNumber that doesn't accurately represent the number of unique active gauges.

To fix this, we should:

  1. Keep track of which gauges have been counted as active in the current epoch.
  2. Only increment activeGaugeNumber if the gauge hasn't been counted as active in this epoch yet.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/Voter.sol#L555

Tool used

Manual Review

nevillehuang commented 3 months ago

Invalid, once claimable is set to zero, activeGaugeNumber will not be incremented anymore