ERC20Gauges: The _incrementGaugeWeight function does not check the gauge parameter enough, so the user may lose rewards.

[code423n4]

Lines of code

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L257

Vulnerability details

Impact

The _incrementGaugeWeight function is used to increase the user's weight on the gauge. However, in the _incrementGaugeWeight function, it is only checked that the gauge parameter is not in _deprecatedGauges, but not checked that the gauge parameter is in _gauges. If the user accidentally uses the wrong gauge parameter, the function will be executed smoothly without any warning, which will cause user loss reward.

    function _incrementGaugeWeight(
        address user,
        address gauge,
        uint112 weight,
        uint32 cycle
    ) internal {
        if (_deprecatedGauges.contains(gauge)) revert InvalidGaugeError();
        unchecked {
            if (cycle - block.timestamp <= incrementFreezeWindow) revert IncrementFreezeError();
        }

        bool added = _userGauges[user].add(gauge); // idempotent add
        if (added && _userGauges[user].length() > maxGauges && !canContractExceedMaxGauges[user])
            revert MaxGaugeError();

        getUserGaugeWeight[user][gauge] += weight;

        _writeGaugeWeight(_getGaugeWeight[gauge], _add, weight, cycle);

        emit IncrementGaugeWeight(user, gauge, weight, cycle);
    }
    ...
    function _writeGaugeWeight(
        Weight storage weight,
        function(uint112, uint112) view returns (uint112) op,
        uint112 delta,
        uint32 cycle
    ) private {
        uint112 currentWeight = weight.currentWeight; // @audit  currentWeight = 0
        // If the last cycle of the weight is before the current cycle, use the current weight as the stored.
        uint112 stored = weight.currentCycle < cycle ? currentWeight : weight.storedWeight; // @audit  stored = 0 < cycle ? 0 : 0
        uint112 newWeight = op(currentWeight, delta); // @audit newWeight = 0 + delta

        weight.storedWeight = stored;
        weight.currentWeight = newWeight;
        weight.currentCycle = cycle;
    }

Proof of Concept

https://github.com/fei-protocol/flywheel-v2/blob/77bfadf388db25cf5917d39cd9c0ad920f404aad/src/token/ERC20Gauges.sol#L257

Tools Used

None

Recommended Mitigation Steps

    function _incrementGaugeWeight(
        address user,
        address gauge,
        uint112 weight,
        uint32 cycle
    ) internal {
-       if (_deprecatedGauges.contains(gauge)) revert InvalidGaugeError();
+       if (_deprecatedGauges.contains(gauge) || !_gauges.contains(gauge)) revert InvalidGaugeError();
        unchecked {
            if (cycle - block.timestamp <= incrementFreezeWindow) revert IncrementFreezeError();
        }

        bool added = _userGauges[user].add(gauge); // idempotent add
        if (added && _userGauges[user].length() > maxGauges && !canContractExceedMaxGauges[user])
            revert MaxGaugeError();

        getUserGaugeWeight[user][gauge] += weight;

        _writeGaugeWeight(_getGaugeWeight[gauge], _add, weight, cycle);

 }

[Joeysantoro]

This is absolutely a valid logic bug. I disagree with the severity, as it would be user error to increment a gauge which was incapable of receiving any weight. Should be medium

[0xean]

This is a tough one to call between medium and high severity. Assets can directly be lost, but putting the wrong address into ANY function call in general is an easy way for a user to lose funds and isn't unique to this protocol. I am going to side with the sponsor and downgrade to medium severity.