New user is able to gain ALL rewards since protocol launch

[c4-bot-1]

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

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

#1194 (marked as duplicate to this issue) contains a test that can be run to confirm

Disputing severity (think it's more fit for medium because there is no loss of user funds, just rewards)

[c4-sponsor]

eswak (sponsor) confirmed

[c4-sponsor]

eswak marked the issue as disagree with severity

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked issue #1194 as primary and marked this issue as a duplicate of 1194

[Trumpero]

I still consider this issue a high severity because the rewards in ProfitManager are matured yield, which should be distributed to the intended users, and the loss of matured yield is considered a high-severity issue based on C4's criteria. @eswak