Slashed staking positions can still claim rewards

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

A user can stake his Credit tokens in SGM, and SGM goes ahead and mints Guild tokens and adds weight to a Gauge that the user chooses. If that Gauge gains profit the user who staked his tokens can claim rewards in the Credit token, in case of loss, the staking position is slashed, i.e. deleted, and the user loses all his stakings. Both the slashing and rewards claiming happen in the same function, getRewards. So in theory if a Guage that a user weighted in lost a certain amount then profited some amount, the user should be slashed and he shouldn't get any rewards from that position.

However, if that happens and getRewards isn't called between the loss and the profit, that user can claim undeserved rewards, where he should've lost everything in that staking position.

Even though getRewards can be called permissionless, i.e. anyone or a bot can call it, however, the protocol shouldn't depend on something like "if the function was anonymously called on time", this case should be handled explicitly.

Proof of Concept

The following test shows the current behavior, where a user claims rewards after being supposedly slashed.

function test_ClaimRewardsAfterSlash() public {
    credit.mint(address(this), 10e18);
    credit.approve(address(sgm), 10e18);
    sgm.stake(term, 10e18);

    assertEq(credit.balanceOf(address(this)), 0);

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

    (
        uint256 lastGaugeLoss,
        SurplusGuildMinter.UserStake memory userStake,
        bool slashed
    ) = sgm.getRewards(address(this), term);

    assertEq(lastGaugeLoss, 0);
    assertEq(userStake.credit, 10e18);
    assertEq(userStake.guild, 20e18);
    assertEq(slashed, false);

    vm.prank(governor);
    profitManager.setProfitSharingConfig(0.5e18, 0, 0.5e18, 0, address(0));
    profitManager.notifyPnL(term, -1);

    // `getRewards` or any other function that calls `getRewards` didn't get the chance to be called

    assertEq(credit.balanceOf(address(this)), 0);

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

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

    (lastGaugeLoss, userStake, slashed) = sgm.getRewards(
        address(this),
        term
    );

    assertEq(lastGaugeLoss, block.timestamp - 13);
    assertEq(userStake.credit, 0);
    assertEq(userStake.guild, 0);
    assertEq(slashed, true);

    assertEq(credit.balanceOf(address(this)), 5e18);
}

The following test shows the normal scenario where getRewards is called between loss and profit, the user lost everything and didn't get any rewards.

function test_ClaimRewardsAfterSlash_normalScenario() public {
    credit.mint(address(this), 10e18);
    credit.approve(address(sgm), 10e18);
    sgm.stake(term, 10e18);

    assertEq(credit.balanceOf(address(this)), 0);

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

    (
        uint256 lastGaugeLoss,
        SurplusGuildMinter.UserStake memory userStake,
        bool slashed
    ) = sgm.getRewards(address(this), term);

    assertEq(lastGaugeLoss, 0);
    assertEq(userStake.credit, 10e18);
    assertEq(userStake.guild, 20e18);
    assertEq(slashed, false);

    vm.prank(governor);
    profitManager.setProfitSharingConfig(0.5e18, 0, 0.5e18, 0, address(0));
    profitManager.notifyPnL(term, -1);

    (lastGaugeLoss, userStake, slashed) = sgm.getRewards(
        address(this),
        term
    );

    assertEq(credit.balanceOf(address(this)), 0);

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

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

    (lastGaugeLoss, userStake, slashed) = sgm.getRewards(
        address(this),
        term
    );

    assertEq(lastGaugeLoss, block.timestamp - 13);
    assertEq(userStake.credit, 0);
    assertEq(userStake.guild, 0);
    assertEq(slashed, true);

    assertEq(credit.balanceOf(address(this)), 0);
}

Tools Used

Manual review + vscode

Recommended Mitigation Steps

The protocol should check if a user is slashed before transferring the rewards. However, this introduces another issue, that is the reverse, if a Gauge profited then lost, the user should still be able to claim the promised rewards, ...

Assessed type

Invalid Validation

[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