Incorrect counter location in a loop can cause core functions to fail

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

1. Inside ERC20Gauges.sol::_incrementGaugeWeight() there is no check for a minimum weight. A weight of 0 can be passed in. Importantly, even though the weight is 0, the gauge will still be added to _userGauges mapping.
2. This causes the gauge with a weight of 0 to be included in the addresses array that will be iterated during ERC20Gauges::_decrementWeightUntilFree() function, which is mandatorily called before any transfer(), burn(), or transferFrom() function for the GuildToken. (Even though the GuildToken will not be transferrable on launch I think, there are plans to have it be transferrable in the future).
3. Importantly, the counter location for ERC20Gauges::_decrementWeightUntilFree() is in the incorrect spot, so if there is a gauge weight of 0, the loop counter does NOT increment because the logic never gets inside of the if (userGaugeWeight != 0) {... block. So the transaction will always run out of gas and revert if there is a gauge with a weight of 0 in getUserGaugeWeight mapping and the user needed to decrement some weight before doing any kind of balance changing function for their GUILD tokens, causing the user to pay all the gas up until the point of the function reverting.

Imagine a scenario where there are 3 gauges and a user has delegated some weight to all 3, importantly with at least one of them being 0 weight:

• _userGauges : [Gauge1, Gauge2, Gauge3]
• getUserGaugeWeight : [Gauge1: 69, Gauge2: 0, Gauge3: 420]

The user has 1000 tokens and wants to transfer their whole balance of 1000 tokens. However, they have already dished out 489 in weight to the gauges. This means they have 511 free tokens available for transfer, so they need to free up 489 tokens so they can transfer out their whole 1000.

The function _decrementWeightUntilFree() will start looping through the users gauges perfectly, but when it encounters Gauge2 with a weight 0, the counter cannot increment because it is in the wrong spot in the for loop. This causes the tx to run until it is out of gas and then revert.

The gauge with 0 weight must be at any position in the addresses array where the loop still needs to free some weight before completing. This breaks transfer(), _burn() and transferFrom() functions from ERC20Gauges.sol and thus GuildToken.sol because they are all prefaced with a call to _decrementWeightUntilFree().

Last thought that may be useful to double check/investigate further: I thought functions like applyGaugeLoss() would always revert when a user has a gauge weight of 0 and needed to free weight because _burn() is called, but it appears this check if (userFreeWeight >= weight) return; in _decrementWeightUntilFree() is always true when called from the context of applyGaugeLoss() , because it always calls _decrementGaugeWeight() and decrements the user's gauge weight before calling _burn(), so there is always more user free weight than the weight passed in.

Proof of Concept

forge test --match-path ./test/unit/tokens/ERC20Gauges.t.sol --match-test testBreakTransferWithZeroWeightIncrement --gas-limit 30000000 -vv

    function testBreakTransferWithZeroWeightIncrement() public {
        // Mint 1000 to an address that can attempt to transfer out Guild tokens.
        token.mint(address(this), 1000e18);

        // Set maximum gauges and add them to the system so the addresses exist.
        token.setMaxGauges(3);
        token.addGauge(1, gauge1);
        token.addGauge(1, gauge2);
        token.addGauge(1, gauge3);

        require(token.incrementGauge(gauge1, 69e18) == 69e18); // arbitrary amount allocated to gauge1
        require(token.incrementGauge(gauge2, 0) == 69e18); // We can increment a gauge by amount weight of 0.
        require(token.incrementGauge(gauge3, 420e18) == 489e18); // arbitrary amount allocated to gauge2
        require(token.userUnusedWeight(address(this)) == 511e18); // 1000 minted to user - 489 delegated = 511 free

        // We try to transfer an amount (1000) that will require us to free some allocated weight from gauges.
        // This causes `_decrementWeightUntilFree()` to loop through to our weight of 0 at gauge2
        // The loop gets infinitely stuck since counter doesn't increment (++i in wrong spot)
        // tx runs out of gas and reverts. Add `--gas-limit 30000000` to test, else it just runs forever.
        // Note: Any transfer amount > 580 fails. (511 + 69). Failure Range: [581, 1000]
        token.transfer(address(1), 1000e18);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Move the counter to the end of the loop, and also make sure gauges cannot be incremented/decremented by 0 with a != 0 check. If this bug happened in production, the user could call ERC20Gauges::_decrementGaugeWieght() and pass in 0 for the weight of that gauge address, and it should properly remove the gauge from the users gauges and fix the issue.

    function _incrementGaugeWeight(address user, address gauge, uint256 weight) internal virtual {
        // Check could also be placed inside `incrementGauge()` instead.
+       require(weight != 0, "ERC20Gauges: zero weight increment");
        ...
    }
    
    function _decrementGaugeWeight(address user, address gauge, uint256 weight) internal virtual {
        // Check could also be placed inside `decrementGauge()` instead.
+       require(weight != 0, "ERC20Gauges: zero weight decrement");
        ...
    }

    /// a greedy algorithm for freeing weight before a token burn/transfer
    /// frees up entire gauges, so likely will free more than `weight`
    function _decrementWeightUntilFree(address user, uint256 weight) internal {
        uint256 userFreeWeight = balanceOf(user) - getUserWeight[user];

        // early return if already free
        if (userFreeWeight >= weight) return;

        // cache totals for batch updates
        uint256 userFreed;
        uint256 totalFreed;

        // Loop through all user gauges, live and deprecated
        address[] memory gaugeList = _userGauges[user].values();

        // Free gauges until through entire list or under weight
        uint256 size = gaugeList.length;
        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;
+            }
        }

        totalWeight -= totalFreed;
    }

Assessed type

Loop

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #152

[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