If a after loss has occured user stakes his CREDIT in SurplusGuildMinter he will get unjustly slashed.

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

Anyone can stake CREDIT tokens in SGM to start voting in a gauge.

function stake(address term, uint256 amount) external whenNotPaused {
    ...
}

This allows outside participation of first loss capital and allows users to participate in the gauge system without exposure to GUILD token price. If the GUILD minted against CREDIT tokens is slashed while voting in a gauge, that CREDIT is seized and donated to the surplus buffer befor computing losses.

This is part of SurplusGuildMinter.getRewards() function:

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

if (slashed) {
            emit Unstake(block.timestamp, term, uint256(userStake.credit));
            userStake = UserStake({
                stakeTime: uint48(0),
                lastGaugeLoss: uint48(0),
                profitIndex: uint160(0),
                credit: uint128(0),
                guild: uint128(0)
            });
            updateState = true;
}

If a new loss occured in a gauge user was voting for, he will get slashed and his CREDIT is seized. This should only be true for a loss that occured after user has started staking, but currently due to an error in the code, user will get slashed even if the loss occured before he started staking. The reasons for this is in getRewards() function:

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

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

As you can see in the first check that determines if a user should get slashed, userStake is not yet defined, meaning lastGaugeLoss will always be greater than userStake.lastGaugeLoss, since latter is always 0 at this point. If there was at least one loss and lastGaugeLoss is not 0, every user to start staking after will get slashed. The impact is a direct loss of funds without a way to avoid this.

Proof of Concept

function testStakeAfterLoss() public {
        address alice = makeAddr("alice");
        credit.mint(alice, 100e18);
        vm.startPrank(alice);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term, 100e18);
        vm.stopPrank();

        credit.mint(address(profitManager), 35e18);
        profitManager.notifyPnL(term, 35e18);

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

        // loss in gauge
        profitManager.notifyPnL(term, -27.5e18);

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

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

        address bob = makeAddr("bob");
        credit.mint(bob, 50e18);
        vm.startPrank(bob);
        credit.approve(address(sgm), 50e18);
        // Bob stakes after a loss
        sgm.stake(term, 50e18);
        assertEq(profitManager.termSurplusBuffer(term), 50e18);
        // Right after bob staked, he is immediately slashed
        (, , bool slashed) = sgm.getRewards(bob, term);
        assertEq(slashed, true);
        sgm.unstake(term, 50e18);
        vm.stopPrank();
        // When he unstakes, he does not receive his CREDIT
        assertEq(credit.balanceOf(bob), 0);
}

Please add this function to SurplusGuildMinter.t.sol and run the test with forge test --match-test 'testStakeAfterLoss' -vv.

Tools Used

Manual review

Recommended Mitigation Steps

In getRewards() function read userStake from state before checking if lastGaugeLoss is greater than userStake.lastGaugeLoss.

lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
// if the user is not staking, do nothing
userStake = _stakes[user][term]; <----- @audit
if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
    slashed = true;
}

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1164

[c4-judge]

Trumpero marked the issue as satisfactory