Attacker can `unstake` and ` cancelUnstake` in the same timestamp manipulating some variables in `StRSR`

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L259

Vulnerability details

The StRSR is putting users in a quote is the user want to withdraw, this prevent an attacker to deposit and withdraw in the same timestamp and perform some manipulations front running a tx and then backruning.

The problem is that users can call unstake frontrunning a tx and then backrun with cancelUnstake to perform manipulation. See impact

Impact

Attacker first have to stake, then he can call freely unstake and cancelUnstake manipulating some important state variable as totalDrafts, draftRSR and the most important stakeRSR with that been said all functions that use this variables can be front runed doing some damage.

See the next scenario:

1. Attacker stake (if user have good amount of money this could be so dangerous) in the StRSR.
2. An honest user want to stake son rtoken.
3. An attacker see this tx in the mempool and he front run this tx unstaking manipulating the stakeRSR
4. The transaction of the honest user go through with a less mint StRSR token than it should be.
5. The attacker just cancel unstake.

Proof of Concept

See the unstake:

    function unstake(uint256 stakeAmount) external {
        _requireNotTradingPausedOrFrozen();
        _notZero(stakeAmount);

        address account = _msgSender();
        require(stakes[era][account] >= stakeAmount, "insufficient balance");

        _payoutRewards();

        // ==== Compute changes to stakes and RSR accounting
        // rsrAmount: how many RSR to move from the stake pool to the draft pool
        // pick rsrAmount as big as we can such that (newTotalStakes <= newStakeRSR * stakeRate)
        _burn(account, stakeAmount); <----

        // newStakeRSR: {qRSR} = D18 * {qStRSR} / D18{qStRSR/qRSR}
        uint256 newStakeRSR = (FIX_ONE_256 * totalStakes + (stakeRate - 1)) / stakeRate;
        uint256 rsrAmount = stakeRSR - newStakeRSR;
        stakeRSR = newStakeRSR; <----

        // Create draft
        (uint256 index, uint64 availableAt) = pushDraft(account, rsrAmount);
        emit UnstakingStarted(index, draftEra, account, rsrAmount, stakeAmount, availableAt);
    }

[Link]

See the cancelUnstake:

 function cancelUnstake(uint256 endId) external {
        _requireNotFrozen();
        address account = _msgSender();

        // We specifically allow cancelling unstaking when undercollateralized
        // require(basketHandler.isReady() && basketHandler.fullyCollateralized(), """);

        uint256 firstId = firstRemainingDraft[draftEra][account];
        CumulativeDraft[] storage queue = draftQueues[draftEra][account];
        if (endId == 0 || firstId >= endId) return;

        require(endId <= queue.length, "index out-of-bounds");

        // Cancelling unstake does not require checking if the unstaking was available
        // require(queue[endId - 1].availableAt <= block.timestamp, "withdrawal unavailable");

        // untestable:
        //      firstId will never be zero, due to previous checks against endId
        uint192 oldDrafts = firstId != 0 ? queue[firstId - 1].drafts : 0;
        uint192 draftAmount = queue[endId - 1].drafts - oldDrafts;

        // advance queue past withdrawal
        firstRemainingDraft[draftEra][account] = endId;

        // ==== Compute RSR amount
        uint256 newTotalDrafts = totalDrafts - draftAmount;
        // newDraftRSR: {qRSR} = {qDrafts} * D18 / D18{qDrafts/qRSR}
        uint256 newDraftRSR = (newTotalDrafts * FIX_ONE_256 + (draftRate - 1)) / draftRate;
        uint256 rsrAmount = draftRSR - newDraftRSR;

        if (rsrAmount == 0) return;

        // Payout rewards before updating draftRSR
        _payoutRewards();

        // ==== Transfer RSR from the draft pool
        totalDrafts = newTotalDrafts; <----
        draftRSR = newDraftRSR; <-----
        emit UnstakingCancelled(firstId, endId, draftEra, account, rsrAmount);

        // Mint new stakes
        mintStakes(account, rsrAmount); <---
    }

[Link]

As you can see there is nothing to prevent call unstake and cancelUnstake in the same timestamp modifying the variables mention in the impact.

The mint and burn function are directly modifying the stakeRSR variable and the other can be see it in the arrows above.

Tools Used

Manual

Recommended Mitigation Steps

Consider Add some mechanism to prevent an user to unstake and cancelUnstake in the same timestamp. Other solution is completely remove cancelUnstake

Assessed type

Error

[thereksfour]

This report lacks sufficient proof and, in contrast to https://github.com/code-423n4/2024-07-reserve-findings/issues/39, does not mention the root cause of the vulnerability.

[jorgect207]

Hey @thereksfour thanks so much for your time.

Instants operation are always being a problem and lead to vulnerabilities, in this report im talking about some risk and manipulation doing stake and then canceling the stake, #39 vuln in finding repo you can see the steps for execute the vuln:

unstakes everything
stakes one token
stakes one token again

Im not mention here the principal root cause since im pointing out different issues here, that why im arguing for a partial 50.

Thanks so much, hope dont bother you

[thereksfour]

This won't happen, unstake reduces stakeRSR while also reducing totalStakes in _burn, so this doesn't make honest users loss.

4. The transaction of the honest user go through with a less mint StRSR token than it should be.

[jorgect207]

Hey @thereksfour let some comments in this new https://github.com/code-423n4/2024-07-reserve-findings/issues/21 medium , my issue looks more similar to the number 21.

Sorry for bother you.