Ethereum Credit Guild - imare's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L221-L234

Vulnerability details

Impact

A lending term in loss will always slash their users even if they stake after the last loss has occurred. Even if the lending term goes trough the offboarded and then onboarded process it will still slash new stakings.

Proof of Concept

When user stakes CREDIT tokens after a lending term has experience lost the stake structure has a field ('UserStake.lastGaugeLoss`) meant to protect this stake

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L141

but in getRewards, the main function that marks if a user should be slashed in case of an occurred loss the UserStake structure is erroneously loaded too late and instead the function uses the structure from the returns clause which is by default set to zero so the following check always returns true and sets the users stake as slashed :

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L221-L234

The following POC (put inside SurplusGuildMinter.t.sol test file) will show a path when a loss is simulated before and after user staking :

    function test_StakeAfterLoss() public {
         // initial state
         assertEq(profitManager.termSurplusBuffer(term), 0);
         assertEq(guild.balanceOf(address(sgm)), 0);
         assertEq(guild.getGaugeWeight(term), 50e18);

         address user1 = address(0x123); // first staker
         address user2 = address(0x124); // second staker
         // stake 100 CREDIT
        
        credit.mint(user1, 100e18);
        credit.mint(user2, 100e18);

        // first users stakes
        vm.startPrank(user1);
        credit.approve(address(sgm), 100e18); 
        sgm.stake(term, 100e18);
        vm.stopPrank();

        SurplusGuildMinter.UserStake memory stake = sgm.getUserStake(user1, term);
        assertEq(uint256(stake.stakeTime), block.timestamp);
        assertEq(stake.lastGaugeLoss, 0); // loss never happened

        (,,bool slashed) = sgm.getRewards(user1, term);
        assertFalse(slashed, "user should not be slashed if no loss");

        assertEq(stake.credit, 100e18);
        assertEq(stake.guild, 200e18); // mintRatio == 2e18

        // simulate loss
        uint256 lossOccuredAt = block.timestamp + 13;
        vm.roll(block.number + 1);
        vm.warp(lossOccuredAt);
        profitManager.notifyPnL(term, -100);

        (,,bool slashed1) = sgm.getRewards(user1, term);
        assertTrue(slashed1, "user should be slashed when loss occurred");

        guild.applyGaugeLoss(term, address(sgm));

        // second user stakes (this could be also the same user)
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        // new user stakes on loss gauge
        vm.startPrank(user2);
        credit.approve(address(sgm), 100e18); 
        sgm.stake(term, 100e18);
        vm.stopPrank();

        SurplusGuildMinter.UserStake memory stake2 = sgm.getUserStake(user2, term);
        assertEq(uint256(stake2.stakeTime), block.timestamp);
        assertEq(stake2.lastGaugeLoss, lossOccuredAt); // loss happend ...so no more zero

        // with last stake user should not be slashed
        (,,bool slashed2) = sgm.getRewards(user2, term);
        assertFalse(slashed2, "user should not be slashed");
    }

The test fails on the last assert with:

[FAIL. Reason: assertion failed] test_StakeAfterLoss() (gas: 800248)
Logs:
  Error: user should not be slashed
  Error: Assertion Failed

indicating the new user stake is not protected as it should be.

Also off- and then on- boarding doesn't help new stakes from slashing because the process (GuildToken.addGauge and GuildToken.removeGauge) doesn't touch any of the term loss fields only the term weights are manipulated.

Tools Used

Manual review

Recommended Mitigation Steps

Move the line that loads the user stake before the lines that check the term loss

    function getRewards(
        address user,
        address term
    )
        public
        returns (
            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
            UserStake memory userStake, // stake state after execution of getRewards()
            bool slashed // true if the user has been slashed
        )
    {
        bool updateState;
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
+       userStake = _stakes[user][term];
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }


        // if the user is not staking, do nothing
-       userStake = _stakes[user][term];
        if (userStake.stakeTime == 0)
            return (lastGaugeLoss, userStake, slashed);

When this patch is applied the previous POC/test succeeds:

[PASS] test_StakeAfterLoss() 

Assessed type

Error