Users can get too many rewards in ProfitManager due to incorrect calculation of userGaugeProfitIndex

[c4-bot-2]

Lines of code

https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/governance/ProfitManager.sol#L416-L427

Vulnerability details

Description

If user weight sets on gauges then they can get a part of the profit if the gauge makes profit. The profits are distributed via the ProfitManager with notifyPnL. Each gauge has a profit index that is stored in the mapping gaugeProfitIndex and increased in the event of a profit. Each user also has a Profit Index. Whenever a user collects his profit, the profit index is set to that of the gauge. Both profit indexes begin at 0. The problem is that when a user sets weight to a gauge for the first time, his profit index remains at 0. This leads to a problem if the gauge the user is settting weight on has already made a profit and therefore the profit index of the gauge is already higher. Since the amount of profit a user gets is determined with _gaugeProfitIndex - _userGaugeProfitIndex, the user would get more profit than he should actually get:
File: src/governance/ProfitManager.sol

427:        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
428:        if (deltaIndex != 0) {
429:            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;

In this way, all profits that existed before the user even put weight on the gauge are calculated for the user.

Impact

User gets part of the profits that were already distributed before he put weight on the gauge. Since these rewards have normally already been distributed to all users who should correctly receive the profits, the new user accidentally gets a part of the surplus buffer without the ProfitManager accounting for it. So it may be that some losses can no longer be modified because there is no longer enough surplus buffer.

Proof of Concept

Here is a POC:

    function testPOC1() public {
        //Setup
        address user1 = address(1337);
        address user2 = address(1338);
        uint256 guildBalanceUser1 = 20e18;
        uint256 guildBalanceUser2 = 20e18;
        uint256 profit = 10e18;
        int256 loss = -10e18;

        vm.startPrank(governor);
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, 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.GUILD_GOVERNANCE_PARAMETERS, address(this));
        profitManager.setProfitSharingConfig(
            0.05e18, // surplusBufferSplit
            0.8e18, // creditSplit
            0.05e18, // guildSplit
            0.1e18, // otherSplit
            address(this) // otherRecipient
        );
        vm.stopPrank();
        guild.mint(user1, guildBalanceUser1);
        guild.mint(user2, guildBalanceUser2);
        guild.setMaxDelegates(4);
        guild.setMaxGauges(4);
        guild.addGauge(1, address(gauge1));

        //POC
        vm.prank(user1);
        guild.incrementGauge(address(gauge1), guildBalanceUser1); //user sets his balance for gauge1

        uint256 surplusBufferSplit = (profit * 0.05e18) / 1e18;
        uint256 guildSplit = (profit * 0.05e18) / 1e18;
        credit.transfer(address(profitManager), profit);
        profitManager.notifyPnL(gauge1, int256(profit)); //profit is distributed for the users who have weight on gauge1
        
        uint256 creditBalancePM = credit.balanceOf(address(profitManager));
        assert(creditBalancePM == surplusBufferSplit + guildSplit); //this shows that the profit manager has the right share of credit tokens after profit

        vm.prank(user1);
        profitManager.claimGaugeRewards(user1, address(gauge1)); //user1 who has weight on gauge1 gets his rewards

        creditBalancePM = credit.balanceOf(address(profitManager));
        assert(creditBalancePM == surplusBufferSplit); //since there is only one user, the entire guild split goes to user1 and the rest remains as a surplus buffer

        vm.startPrank(user2);
        guild.incrementGauge(address(gauge1), guildBalanceUser2); //user2 now also sets weight to gauge1
        profitManager.claimGaugeRewards(user2, address(gauge1)); //user2 gets rewards even though he only sets weight to gauge1 after profit has been notified. 
        //However, since everything has actually already been distributed to the rightful beneficiaries, User2 incorrectly receives what should be available as 
        //surplus buffer.
        vm.stopPrank();

        creditBalancePM = credit.balanceOf(address(profitManager));
        assert(creditBalancePM == 0); //this shows that the profit manager no longer has a surplus buffer
        
        vm.expectRevert("ERC20: burn amount exceeds balance"); //no loss can be notified anymore because it would revert because the surplus buffer is at user2
        profitManager.notifyPnL(gauge1, loss);
    }

This test can be added to the file test/unit/governance/ProfitManager.t.sol and started with this command: forge test --match-test testPOC1

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

When a user sets weight to a gauge for the first time, their profit index should be set to that of the gauge so as not to mistakenly get any of the previous profits.
File: src/governance/ProfitManager.sol

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

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