Ethereum Credit Guild - deliriusz's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L409-L436

Vulnerability details

Gauge rewards in ECG are similar to Compound - there is a global ever-increasing profit index, and one profit index per user, synchronized by applying the delta between gaugeProfitIndex and userGaugeProfitIndex.

When user profit index is not initialized, it's set to 1e18. This is an issue when the ProfitManager operates for some time and the gaugeProfitIndex grew to a new value. Then we have following situation:

gaugeProfitIndex = 1.2e18 User just joined, userGaugeProfitIndex is set to 1e18. Hence He's elligible to all rewards since gauge started operating.

The problem is even worse if the Guild token transfer are enabled. Then a malicious actor can use sybil accounts and claim all the rewards of other users to themselves.

Impact

All rewards are stolen by sybil accounts, able to claim all rewards since the protocol launch.

Proof of Concept

1. Wait for gaugeProfitIndex to grow.
2. Call GuildToken.incrementGauge(). From a new account. This will internally call ProfitManager.claimGaugeRewards().
3. Because the user didn't claimed their profit before, their profit will be initialzed to 1e18, allowing them to claim all rewards since beginning.
4. If Guild token transfers are enabled, the user can get the tokens back, transfer the amount to their account and repeat the attack.

Tools Used

Manual analysis

Recommended Mitigation Steps

In case that the user doesn't have index assigned, assign it to current gauge profit index and return from the function early:

    function claimGaugeRewards(
        address user,
        address gauge
    ) public returns (uint256 creditEarned) {
        uint256 _userGaugeWeight = uint256(
            GuildToken(guild).getUserGaugeWeight(user, gauge)
        );
        if (_userGaugeWeight == 0) {
            return 0;
        }
        uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
        uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
        if (_gaugeProfitIndex == 0) {
            _gaugeProfitIndex = 1e18;
        }
        if (_userGaugeProfitIndex == 0) {
-            _userGaugeProfitIndex = 1e18;
+            userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
+            return 0;

Assessed type

Math

Lines of code

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

Vulnerability details

When a loss is recorded in the system, it's shared across all gauge voters, and in order to perform any action on them, GuildToken.applyGaugeLoss() has to be called. However, this function will run indefinitely when it meets getUserGaugeWeight[user][gauge] that is 0. This is easy to achieve by increasing voting weight by 0 by user. Then, the loop will run indefinitely, because of mismatched ++i position. This makes the position loss unappliable:

        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;
                }
            }
        }

Impact

User GUILD tokens which are menat to be burned after the protocol, leaving "poisonous" GUILD tokens in a gauge forever.

Tools Used

Manual analysis

Recommended Mitigation Steps

The fix is very straight forward:

        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;
                }
-            }
        }

Generally, micro optimizations are an enemy to the security and should be verified very carefully.

Assessed type

Error