Users will always be slashed if gauge has incurred any loss

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L229

Vulnerability details

Impact

SurplusGuildMinter enables users to stake their CREDIT tokens in order to support gauges, obtaining rewards in the form of GUILD and CREDIT tokens in exchange. This is thought as a safety mechanism: users who stake in favor of a properly-behaved lending term will get rewarded, while users who stake for a gauge that causes losses to the protocol will get their staked balance slashed.

In order to apply slashing in the staking mechanism, the getRewards() function is used, which will

1. Fetch the lastGaugeLoss() of the term being staked for. The last gauge loss is the timestamp of the last loss that impacted the gauge.
2. If the user’s lastGaugeLoss timestamp is smaller than the gauge’s lastGaugeLoss(), it will mean that the user is slashable (user’s lastGaugeLoss is set to the staking timestamp, meaning that if a loss occurs AFTER the user has staked, the user will be slashable)

// SurplusGuildMinter.sol
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);

        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { // @audit-issue [HIGH] - Users will always be slashed if gauge has incurred any loss
            slashed = true;
        }

             // if the user is not staking, do nothing
        userStake = _stakes[user][term];

                ...

}

The problem with the getRewards() is that the user staked status stored in the contract’s storage is actually fetched AFTER performing the slashing check. The previous code snippet shows how _stakes[user][term] is loaded into the memory variable userStake after checking the slashing status. userStake.lastGaugeLoss is used in order to verify if the user is slashable (if (lastGaugeLoss > uint256(userStake.lastGaugeLoss))), but its value will always be zero because its actual data stored in storage will be obtained later and userStake is never initialized (it is a named return parameter).

Users staking after a gauge loss should not be slashable (only users that were staking prior or during a loss should be elegible of being slashed). However, the previously described issue makes ALL stakers that stake after any loss incurred by the gauge be automatically slashable, effectively losing their all of their staked CREDIT tokens.

Proof of Concept

The following proof of concept shows how user2 will lose all of their staked balance due to the previous issue. In order to reproduce the PoC, just copy the function into ./test/unit/loan/SurplusGuildMinter.sol and execute it.

function testVuln_usersWillAlwaysGetSlashedAfterGaugeHasIncurredALoss() public {
        // initial state
        address user1 = makeAddr("user1"); 
        address user2 = makeAddr("user2"); 
        credit.mint(address(user1), 100e18);
        credit.mint(address(user2), 100e18);

        // 1. User 1 stakes 100 CREDIT
        vm.startPrank(user1);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term, 100e18);

        // check state after staking
        SurplusGuildMinter.UserStake memory stakeUser1 = sgm.getUserStake(user1, term);
        assertEq(stakeUser1.credit, 100e18); 
        vm.stopPrank();

        // 2. Move forward one block and notify a loss to the gauge
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        profitManager.notifyPnL(term, -10e18);
        assertEq(guild.lastGaugeLoss(term), block.timestamp);

        // --- After this point, any user staking will always be slashed --- // 

        // 3. Move forward another block and slash Surplus Guild Minter contract
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        guild.applyGaugeLoss(term, address(sgm));
        assertEq(guild.balanceOf(address(sgm)), 0); // slashed

        // 4. Stake with user 2.

        // Initially, user 2 will stake 100 CREDIT tokens. 
        vm.startPrank(user2);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term, 80e18);

        // At this point, getRewards() already computes that user2 is slashable. However, because staking for the first time
        // does not check if user must be slashed, user2's first stake will be succesful, returning a user stake of 100 CREDIT.
        SurplusGuildMinter.UserStake memory stakeUser2 = sgm.getUserStake(user2, term);
        assertEq(stakeUser2.credit, 80e18); 

        // From now on, if a `getRewards()` call is performed, it will not return earlier and will check the slashing state of the user due 
        // to `userStake.stakeTime`not being == 0. This will make `getRewards()` slash the user, and the previously staked 100 CREDIT tokens
        // will be completely slashed, making user2 have a staked balance of 0 CREDIT (being slashed)
        sgm.getRewards(user2, term);

        stakeUser2 = sgm.getUserStake(user2, term);
        assertEq(stakeUser2.credit, 0); 
        vm.stopPrank();
    }

Tools Used

Manual review, foundry

Recommended Mitigation Steps

The mitigation is fairly easy. The user stake stored in storage needs to be loaded to userStake prior to executing the slashing check, likeso:

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);

+        // if the user is not staking, do nothing
+        userStake = _stakes[user][term];

        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { 
            slashed = true;
        }

-        // if the user is not staking, do nothing
-        userStake = _stakes[user][term];

Assessed type

Other

[0xSorryNotSorry]

The issue is well demonstrated, properly formatted, contains a coded POC. Marking as HQ.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1164

[c4-judge]

Trumpero marked the issue as satisfactory