Infinite loop in ERC20Gauges::_decrementWeightUntilFree when one of the user's gauges has a zero weight

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20Gauges.sol#L532-L534

Vulnerability details

Impact

The position of ++i in the loop of the ERC20Gauges::_decrementWeightUntilFree function is incorrect, and can result in an infinite loop.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20Gauges.sol#L500-L539

    function _decrementWeightUntilFree(address user, uint256 weight) internal {
...
        for (
            uint256 i = 0;
            i < size && (userFreeWeight + userFreed) < weight;
        ) {
            address gauge = gaugeList[i];
            uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
            if (userGaugeWeight != 0) {
                userFreed += userGaugeWeight;
                _decrementGaugeWeight(user, gauge, userGaugeWeight);

                // If the gauge is live (not deprecated), include its weight in the total to remove
                if (!_deprecatedGauges.contains(gauge)) {
                    totalTypeWeight[gaugeType[gauge]] -= userGaugeWeight;
                    totalFreed += userGaugeWeight;
                }

@>              unchecked {
@>                  ++i;
@>              }
            }
        }

When userGaugeWeight == 0, i is not incremented and will not break out of the for loop. This would only happen if a user increments their gauge to zero. If they then transfer or burn their tokens, the transaction would fail.

Proof of Concept

Test case in ERC20Gauges.t.sol

    function testAudit_IncrementGauges() public {
        token.mint(address(this), 100e18);

        token.setMaxGauges(2);
        token.addGauge(1, gauge1);
        token.addGauge(1, gauge2);

        // User calls incrementGauge with a weight of 0.
        token.incrementGauge(gauge1, 0);
        token.incrementGauge(gauge2, 100e18);

        require(token.getUserGaugeWeight(address(this), gauge1) == 0);
        // Transfers tokens. This results in the infinite loop and the transaction will fail.
        token.transfer(address(1), 100e18);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Move ++i outside of the if (userGaugeWeight != 0) block

        for (
            uint256 i = 0;
            i < size && (userFreeWeight + userFreed) < weight;
        ) {
            address gauge = gaugeList[i];
            uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
            if (userGaugeWeight != 0) {
                userFreed += userGaugeWeight;
                _decrementGaugeWeight(user, gauge, userGaugeWeight);

                // If the gauge is live (not deprecated), include its weight in the total to remove
                if (!_deprecatedGauges.contains(gauge)) {
                    totalTypeWeight[gaugeType[gauge]] -= userGaugeWeight;
                    totalFreed += userGaugeWeight;
                }
-               unchecked {
-                   ++i;
-               }
            }
+           unchecked {
+               ++i;
+           }
        }

Assessed type

Loop

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

eswak marked the issue as disagree with severity

[eswak]

While this is a valid issue, I think it should be QA because I don't think there is a way to exploit this.

The user would need to purposefully increment 0 weight on a gauge (through etherscan directly and not through our UI), and then it would only prevent them from transferring their tokens until they decrement 0 weight.

I initially thought this could be used to prevent slashing (which would have higher severity), but this is not the case because applyGaugeLoss calls _decrementGaugeWeight before calling _burn (that itself calls _decrementWeightUntilFree, where the bug is).

[c4-sponsor]

eswak (sponsor) confirmed

[Trumpero]

Agree with the sponsor that this issue should be a QA/low since it can't prevent slashing. applyGaugeLoss calls _decrementGaugeWeight before calling _burn, so _decrementWeightUntilFree will return from the start (code snippet).
This issue may only break the transfer/burn of a user when they increment 0 weight to a gauge. In such cases, it's the user's mistake, and they can recover by adding/allocating weight greater than 0 again

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-a

[c4-judge]

Trumpero marked the issue as grade-c