CREDIT holders cannot stake in the same block a lending term has experienced a loss

[c4-bot-2]

Lines of code

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

Vulnerability details

Description

Users who have CREDIT can stake their CREDIT to vote for a lending term using the SurplusGuildMinter. When the term they vote for experiences a loss they forfeit their CREDIT and may only receive a CREDIT reward. This is enforced by the getReward function in SurplusGuildMinter. Anyone can call this function and when it is called it sends stakers their rewards and also applies their 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);
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }

        ...

        // If a loss occurred while the user was staking, the GuildToken.applyGaugeLoss(address(this))
        // can be called by anyone to slash address(this) and decrement gauge weight etc.
        // The contribution to the surplus buffer is also forfeited.
        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;
        }

        // store the updated stake, if needed
        if (updateState) {
            _stakes[user][term] = userStake;
        }
    }

getRewards uses the slashed variable to determine if a term's (gauge) loss should be applied to a user or not. The first if statement sets the value of slashed to true if lastGaugeLoss > uint256(userStake.lastGaugeLoss) . Where lastGaugeLoss is the last time the user recorded a loss and userStake.lastGaugeLoss is the last time the gauge experienced a loss as at the time the user last called stake. So if the gauge does not experience any loss between when the user last staked and when getRewards is called he isn't slashed, else he is slashed.

In the stake function below, getRewards is first called so it applies pending rewards and losses to the msg.ender (user). The require statement after it does not let the user stake if there is a gauge loss in that block. The reason for the require statement must be because the stake function resets userStake.lastGaugeLoss to the lastGaugeloss returned by getRewards. If there is a loss in the block and the user stakes, he may be able to call stake and escape the check in getRewards that will set slashed to true. The issue is the initial call to getRewards already applies the losses regardless of if it happened in that block. This makes the require statement redundant.

It's important to note that there are two set of users that may call stake. A user who has already staked and a user who is staking for the first time. The require statement should be applicable to only the user who is staking again but reverts the transaction for both users. This causes a Denial Of Service.

function stake(address term, uint256 amount) external whenNotPaused {
        // apply pending rewards
        (uint256 lastGaugeLoss, UserStake memory userStake, ) = getRewards(
            msg.sender,
            term
        );

        require(
            lastGaugeLoss != block.timestamp,
            "SurplusGuildMinter: loss in block"
        );

        ...

        // update state
        userStake = UserStake({
            stakeTime: SafeCastLib.safeCastTo48(block.timestamp),
            lastGaugeLoss: SafeCastLib.safeCastTo48(lastGaugeLoss),
            profitIndex: SafeCastLib.safeCastTo160(
                ProfitManager(profitManager).userGaugeProfitIndex(
                    address(this),
                    term
                )
            ),
            credit: userStake.credit + SafeCastLib.safeCastTo128(amount),
            guild: userStake.guild + SafeCastLib.safeCastTo128(guildAmount)
        });
        _stakes[msg.sender][term] = userStake;

        // emit event
        emit Stake(block.timestamp, term, amount);
    }

Impact

All calls to stake will revert when the term being staked in experiences a loss in the block stake is called.

Proof of Concept

Alice cannot stake in a block that has experienced a loss even when the loss in that block has already been applied to her stake.

The code can be run in SurplusGuildMinter.t.sol.

function testStakeWithLoss() public{
    // prepare
    SurplusGuildMinter guildMinter = sgm;
    address Alice = vm.addr(1);
    credit.mint(Alice, 100e18);

    // stake
    vm.startPrank(Alice);
    credit.approve(address(sgm), 100e18);
    guildMinter.stake(term, 90e18);
    vm.stopPrank();

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

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

    // stake in loss block
    vm.expectRevert("SurplusGuildMinter: loss in block");
    guildMinter.stake(term, 10e18); // getRewards is called here but it still reverts

    SurplusGuildMinter.UserStake memory stake = guildMinter.getUserStake(Alice, term);
    assertEq(stake.credit, 90e18);

    // even when getRewards applies the loss it still reverts
    guildMinter.getRewards(Alice, address(term)); // applies loss
    stake = guildMinter.getUserStake(Alice, term);
    assertEq(stake.credit, 0);
    vm.expectRevert("SurplusGuildMinter: loss in block");
    guildMinter.stake(term, 10e18);
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove the require statement as it is redundant and causes a DOS.

Assessed type

DoS

[0xSorryNotSorry]

I couldn't confirm a DOS but a revert as per the blocktime only which is expected.

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[Trumpero]

require(
          lastGaugeLoss != block.timestamp,
          "SurplusGuildMinter: loss in block"
      );

This validation is correct to avoid missing losses

[c4-judge]

Trumpero marked the issue as unsatisfactory: Invalid

[nonseodion]

Hi @Trumpero thanks for the good work so far. I'd like to address the comments here.

Presort says:

I couldn't confirm a DOS but a revert as per the blocktime only which is expected.

and

Judge says:

require(
          lastGaugeLoss != block.timestamp,
          "SurplusGuildMinter: loss in block"
      );

This validation is correct to avoid missing losses

I disagree with these two statements here's why:

1. The "This validation is correct to avoid missing losses" argument.

   In the call to stake(), getRewards() is first called and this applies rewards and any losses that may have occurred in the lending term during the time the user may have staked. This occurs before the validation.

   Please take a look at this section of the report for more details on how the loss is applied in getRewards():

   getRewards uses the slashed variable to determine if a term's (gauge) loss should be applied to a user or not. The first if statement sets the value of slashed to true if lastGaugeLoss > uint256(userStake.lastGaugeLoss) . Where lastGaugeLoss is the last time the user recorded a loss and userStake.lastGaugeLoss is the last time the gauge experienced a loss as at the time the user last called stake. So if the gauge does not experience any loss between when the user last staked and when getRewards is called he isn't slashed, else he is slashed.

   By the time execution gets to the validation in the stake() function, any losses incurred by the lending term have already been applied to the user and there is no need to revert the transaction.

   This makes the validation unnecessary.

2. The " I couldn't confirm a DOS" argument.

   Two types of users may call stake().

   • Users who currently don't have any stake in the lending term and
   • Users who have staked in the lending term.

   The validation is meant to be applied to only the second set of users but it also applies to the first which causes an unnecessary temporary Denial of Service for the first set of users.

   Considering the first argument, the DOS also extends to the second set of users because any loss they may have incurred has already been applied making the revert unnecessary.

The finding addresses why the validation was added and why it is not needed. Please check this section for more details:

The reason for the require statement must be because the stake function resets userStake.lastGaugeLoss to the lastGaugeloss returned by getRewards. If there is a loss in the block and the user stakes, he may be able to call stake and escape the check in getRewards that will set slashed to true. The issue is the initial call to getRewards already applies the losses regardless of if it happened in that block. This makes the require statement redundant.

Considering that the revert may occur more than once across multiple blocks to multiple users staking on different lending terms when it can be avoided, is enough to make this a valid finding.

This finding will make the core staking functionality unavailable multiple times at different periods to multiple users.

For this reason, the MEDIUM severity should be accepted for this report.

[Trumpero]

@nonseodion Regarding my statement that "This validation is correct to avoid missing losses": If the protocol allows staking in the same block with losses (as your recommendation), there is a case where if a staking is created between two loss notifications in the same block, and it won't be slashed. Because of the validation lastGaugeLoss > uint256(userStake.lastGaugeLoss) to set slashed to true, that stake won't be slashed even when there is a loss that happens after it. This's the reason why the protocol doesn't allow staking in the same block with losses. So this issue should be invalid.