Diminished returns for gaugeWeight voters if they stake before applyGaugeLoss for all slashable stakers

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L147

Vulnerability details

Proof of Concept

A Gauge weight can be increased in 2 ways: directly, by GUILD holders by calling guild.incrementGauge and by SurplusGuildMinter(SGM) stakers who
stake CREDIT (sent to ProfitManager - used as first-loss capital) and minting Guild (hold by SGM address).

Slashing events can happen from 2 reasons:

• A. a borrower missed a partial repayment or
• B. a lendingTerm is off boarded.

When bad debt is realized:

• C. CREDIT is burned;
• D. GUILD is not burned and gauge weight is not decremented immediately;

Regarding (D), for gauge weight to be decremented, guild.applyGaugeLoss is expected to be called for all users who voted for that gauge.
The users who increase gaugeWeight after notifyGaugeLoss (t0) is called and before last user is slashed via applyGaugeLoss (t1) will get
diminished guildSplit returns for the profit accumulated in the same [t0, t1] interval.
The reason for this lies in how gaugeProfitIndex is calculated:

gaugeProfitIndex[gauge] = _gaugeProfitIndex + (amountForGuild * 1e18) / _gaugeWeight;

_gaugeWeight does not reflect the real, updated gaugeWeight until applyGaugeLoss is called for all gauge weight voting users.
Even if it's known that applyGaugeLoss must be called to decrement gauge weight for slashed users the risk of diminished returns was not considered.
Users who allocate gaugeWeight are exposed to slashing but don't get their fair share of interest.

Coded PoC:

Add following test in GuildToken.t.sol file and run it with forge test --mt testDiminishedGuildSplitReturns -vvv
NOTE: SGM.getRewards must be updated, otherwise other bug will interfere with this test: cache userStake = _stakes[user][term]; before comparing it with lastGaugeLoss

        // note: `SGM.getRewards` must be updated, otherwise other bug will interfere with this test:
        // cache `userStake = _stakes[user][term];` before comparing it with `lastGaugeLoss`


         function testDiminishedGuildSplitReturns() public {
            _addFundsToSurplusBufferHelper();
            guild.decrementGauge(term, uint112(50e18));// undo setUp() gauge weight voting
            assertEq(profitManager.surplusBuffer(), 20e18, "surplus buffer");

            vm.prank(governor);
            profitManager.setProfitSharingConfig(
                0, // surplusBufferSplit
                0, // creditSplit
                1e18, // 100% on guildSplit 
                0, // otherSplit
                address(0) // otherRecipient
            );

            address alice = makeAddr("alice");
            address bob = makeAddr("bob");

            guild.mint(alice, 100e18);
            vm.startPrank(alice);
            //credit.approve(address(sgm), 100e18);
            guild.incrementGauge(term, 100e18);
            vm.stopPrank();

            vm.warp(block.timestamp + 13);
            vm.roll(block.number + 1);

            assertEq(guild.balanceOf(address(sgm)), 60e18, "sgm balance");// 2* amount staked for term2
            assertEq(guild.getGaugeWeight(term), 100e18, "gauge weight after alice incrementGauge(100)");
                
            profitManager.notifyPnL(term, -10e18);
            assertEq(profitManager.surplusBuffer(), 10e18, "surplus buffer");
            //note: GuildToken.applyGaugeLoss(alice) is not called for a 
            //few blocks for whatever reason (high gas fees, low activity marked, many gaugeWeight voters etc)
            assertEq(guild.getGaugeWeight(term), 10e18, "gauge weight after notifyPnL(-10e18)");

            // you can't increaseWeight if you had a loss in same block => next block
            vm.warp(block.timestamp + 13);
            vm.roll(block.number + 1);

            credit.mint(bob, 100e18);
            vm.startPrank(bob);

            credit.approve(address(sgm), 100e18);
            sgm.stake(term, 100e18);
            vm.stopPrank();

            assertEq(credit.balanceOf(bob), 0, "bob balance after staking");
            assertEq(guild.getGaugeWeight(term), 200e18, "gauge weight after bob stake 100");//MINT_RATIO * 100
                
            credit.mint(address(profitManager), 10e18);
            profitManager.notifyPnL(term, 10e18);
            console.log("lastGaugeLoss", guild.lastGaugeLoss(term));
            console.log("lastGaugeLossApplied", guild.lastGaugeLossApplied(term, alice));
            // finally applyGaugeLoss() is called and gaugeWeight is decreased
            guild.applyGaugeLoss(term, alice);

            vm.prank(bob);
            sgm.unstake(term, 100e18);// unstake entire amount

            assertEq(credit.balanceOf(bob), 100e18 + 10e18,"bob balance after unstaking"); // principal + 100% rewards
            assertEq(guild.getGaugeWeight(term), 0);
        }

        function _addFundsToSurplusBufferHelper() private {
            // only to have an amount in surplusBuffer and start from a easy-to-follow state
            address charlie = makeAddr("charlie");
            address term2 = address(new MockLendingTerm(address(core)));
            guild.addGauge(1, term2);

            credit.mint(charlie, 30e18);
            vm.startPrank(charlie);
            credit.approve(address(sgm), 30e18);
            sgm.stake(term2, 30e18);
            vm.stopPrank();
            profitManager.notifyPnL(term2, -10e18);
        }

Tools Used

Manual review

Recommended Mitigation Steps

Consider disallow gaugeWeight voting until all addresses have been slashed (via applyGaugeLoss).

Assessed type

Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #956

[c4-judge]

Trumpero marked the issue as duplicate of #262

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-b

[c4-judge]

Trumpero marked the issue as grade-c