santipu_ - DoS on liquidations when utilization rate is high

[sherlock-admin2]

santipu_

high

DoS on liquidations when utilization rate is high

Summary

When a position is liquidated, the liquidator seizes some (or all) of the borrower's assets in compensation for repaying the unhealthy debt. However, when the utilization rate is high in a market, liquidations won't work because of insufficient protocol liquidity.

An attacker could use this bug to frontrun a liquidation transaction by withdrawing assets from a market, bringing the utilization higher and preventing the liquidation.

Vulnerability Detail

In liquidation, one of the last steps is to seize the assets from a borrower and give them to the liquidator. The seize function calls internalSeize to seize the assets from the borrower:

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L678-L694

  function internalSeize(Market seizeMarket, address liquidator, address borrower, uint256 assets) internal {
    if (assets == 0) revert ZeroWithdraw();

    // reverts on failure
    auditor.checkSeize(seizeMarket, this);

    RewardsController memRewardsController = rewardsController;
    if (address(memRewardsController) != address(0)) memRewardsController.handleDeposit(borrower);
    uint256 shares = previewWithdraw(assets);
>>  beforeWithdraw(assets, shares);

    // ...
  }

The function internalSeize, in turn, calls beforeWithdraw to update the state of the market before the actual seizing of the assets. The issue is that beforeWithdraw checks if the protocol has enough liquidity for the withdrawal of assets:

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L698-L706

  function beforeWithdraw(uint256 assets, uint256) internal override whenNotPaused {
    updateFloatingAssetsAverage();
    depositToTreasury(updateFloatingDebt());
    uint256 earnings = accrueAccumulatedEarnings();
    uint256 newFloatingAssets = floatingAssets + earnings - assets;
    // check if the underlying liquidity that the account wants to withdraw is borrowed
>>  if (floatingBackupBorrowed + floatingDebt > newFloatingAssets) revert InsufficientProtocolLiquidity();
    floatingAssets = newFloatingAssets;
  }

This check will make the whole liquidation revert when the utilization rate of that market is near the top. An attacker can use this bug to prevent a liquidation of one of his accounts by frontrunning the liquidation and withdrawing liquidity with another account. When that liquidity is withdrawn, the actual liquidation will fail.

Impact

When the utilization rate of a market is high, the liquidations will fail, causing bad debt on the protocol if the price moves against the borrower. Liquidations are a core invariant of any lending protocol and should never fail in order to prevent bad debt, and ultimately, a bank run.

An attacker can use this vulnerability to make his positions not liquidatable by frontrunning a liquidation and withdrawing liquidity from that market with another account.

PoC

The following PoC can be pasted in the Market.t.sol file and can be run with the following command forge test --match-test test_fail_liquidation.

function test_fail_liquidation() external {
    // We set the price of the asset to 0.0002 (1 ETH = 5,000 DAI)
    daiPriceFeed.setPrice(0.0002e18);

    // Simulate deposits on the markets
    market.deposit(50_000e18, ALICE);
    marketWETH.deposit(10e18, address(this));

    // Simulate borrowing on the markets
    vm.startPrank(ALICE);
    market.auditor().enterMarket(market);
    marketWETH.borrow(5e18, ALICE, ALICE);
    vm.stopPrank();

    market.borrow(35_000e18, address(this), address(this));

    // Price falls to 0.00025 (1 ETH = 4,000 DAI)
    daiPriceFeed.setPrice(0.00025e18);

    // Position cannot be liquidated due to insufficient protocol liquidity
    vm.prank(BOB);
    vm.expectRevert(InsufficientProtocolLiquidity.selector);
    market.liquidate(address(this), type(uint256).max, marketWETH);
}

Code Snippet

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/Market.sol#L704

Tool used

Manual Review

Recommendation

To mitigate this issue is recommended to not call beforeWithdraw in a liquidation and add the logic of beforeWithdraw in the internalSeize function except for the liquidity check.

[MehdiKarimi81]

Escalate

Consider: There is a reserve factor that prevents borrowing all of the pool assets, For borrowing, the attacker needs to deposit some collateral and since the utilization rate is high, the interest rate would be high and the borrower should pay a huge interest and it doesn't seem to have any profit for the attacker, The liquidator can choose to liquidate from a different market that the user has entered or only liquidate some part of the assets Users deposits add to pool liquidity and prevent DoS

I believe it should be invalid or medium

[sherlock-admin3]

Escalate

Consider: There is a reserve factor that prevents borrowing all of the pool assets, For borrowing, the attacker needs to deposit some collateral and since the utilization rate is high, the interest rate would be high and the borrower should pay a huge interest and it doesn't seem to have any profit for the attacker, The liquidator can choose to liquidate from a different market that the user has entered or only liquidate some part of the assets Users deposits add to pool liquidity and prevent DoS

I believe it should be invalid or medium

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[santichez]

@santipu03 given the POC you provided, if the liquidity check is removed from the liquidation flow, the liquidation will still fail because there are not enough underlying assets to be withdrawn. If the utilization rate is high and a user front-runs the liquidation by withdrawing floating pool deposits from another account, the liquidation will still revert due to insufficient underlying assets and this is a problem we can't escape, same occurs with AAVE or Compound. That's why adjust factor values are important, which might give enough time for the market to arbitrage the rates by itself (other users providing liquidity) and liquidations happening sooner. The liquidity check also ensures that only floating pool deposits are being used when seizing users' collateral. Let's say there's an excess of fixed pool deposits and we don't have the liquidity check, then possibly, the underlying assets of the fixed pool deposits are going to be withdrawn and this is unwanted.

[0x73696d616f]

@santipu03 given the POC you provided, if the liquidity check is removed from the liquidation flow, the liquidation will still fail because there are not enough underlying assets to be withdrawn.

This is not true as the protocol can first pay off the debt using the liquidator's assets (it currently does not and it is incorrect, should be fixed). When it does the liquidity check, it uses updated debt values (removed the debt first), but has not yet pulled the debt assets, which is incorrect.

[santichez]

@0x73696d616f I have just seen your report that I previously missed because it was marked as duplicate and I consider it more valid indeed. I'll jump back to your report asap. My answer to @santipu03 is not inaccurate though, since in the POC we are dealing with a cross-asset liquidation.

[santipu03]

The fix proposed on #118 will also fix the scenario described in the report on some cases. However, it's still possible (and probable) that liquidations may fail due to the liquidity check when the utilization rate is high.

It's true that even if the liquidity check is removed the transaction would fail due to insufficient funds on the contract. However, the fact that an attacker can make liquidations consistently revert by withdrawing assets is directly breaking a core invariant of a lending protocol. Moreover, this scenario can be given without the need of an attacker, when the market is highly utilized, liquidations will also fail.

[etherSky111]

The impact of this issue is medium, but the likelihood is low. It can only occur when almost all liquidity is borrowed and the seize market is the same as the borrow market. Therefore, I believe this issue can be classified as low/medium.

[0x73696d616f]

The impact of this issue is medium

The impact of DoSing liquidations is high in lending protocols, even more so when users can trigger it.

but the likelihood is low.

I disagree with this, borrowing most of the liquidity is an expected and accepted state of a lending protocol, so the likelihood is not low.

[etherSky111]

@Trumpero , could you please also take a look at this as LSW?

I believe that @MehdiKarimi81 described correctly in the escalation. This is really rare case and easy to avoid this as the liquidator(or bot) can choose other market as seize market or partially liquidate.

[Trumpero]

From my perspective, some protocols have this risk as well, and I thought this was an accepted risk, so I didn't submit this issue. For example, you can look at the Silo protocol, which has the concept of 'deposit-only' assets that can't be borrowed. However, if the market utilization becomes high, these 'deposit-only' assets can still be borrowed. So, I think the likelihood of high utilization is quite small

But I don't negate the possibility of the issue. I think we should leave it to the head judge to decide

[santipu03]

Even though the likelihood of this issue occurring organically might be medium/low, an attacker can exploit this vulnerability to always prevent getting liquidated.

The attack sequence is the following:

1. Attacker deposits collateral at market A (better if it's highly utilized but it's not necessary)
2. Attacker deposits a ton of collateral with another account at market A.
3. Attacker borrows from whatever market using the first account.
4. When the attacker is going to be liquidated, frontrun txn by withdrawing assets from market A using the second account. Liquidation will fail because of the lack of liquidity.

Note that the attacker can withdraw the maximum liquidity allowed so that the liquidator cannot even liquidate partially. Also, the liquidator cannot seize from another market because the attacker only has collateral at market A. The cost of the attack is low because there's no fee on deposits or withdrawals of collateral, the only cost is the gas spent.

As you can see, the impact is high (liquidation constantly reverts) and the likelihood is also high (the attacker can execute the attack as long as necessary). It perfectly fits in the high severity category according to the Sherlock rules:

Definite loss of funds without (extensive) limitations of external conditions.

[0x73696d616f]

Given the fact that bad debt may be socialized by subtracting from the earnings accumulator, there is no need to perform the liquidity check. It just creates more bad debt by delaying the problem further. The correct flow is, if the market is close to not having enough liquidity for liquidations, liquidate the user and socialize the bad debt, if this is the case. It makes no sense to accumulate more bad debt.

If we look at the exact liquidity check:

uint256 earnings = accrueAccumulatedEarnings();
uint256 newFloatingAssets = floatingAssets + earnings - assets;
// check if the underlying liquidity that the account wants to withdraw is borrowed
if (floatingBackupBorrowed + floatingDebt > newFloatingAssets) revert InsufficientProtocolLiquidity();

The liquidity is checked against accrueAccumulatedEarnings(). There is room to deduct losses from the earnings accumulator, which is the correct flow to fix the risk.

[0x73696d616f]

This part here is key

the liquidator cannot seize from another market because the attacker only has collateral at market A.

The attacker is able to limit the losses of the total position to only a portion of the total collateral (both accounts). Even on market A alone, the only assets that may be seized are that of the account that is borrowing.

[etherSky111]

Even though the likelihood of this issue occurring organically might be medium/low, an attacker can exploit this vulnerability to always prevent getting liquidated.

The attack sequence is the following:

1. Attacker deposits collateral at market A (better if it's highly utilized but it's not necessary)
2. Attacker deposits a ton of collateral with another account at market A.
3. Attacker borrows from whatever market using the first account.
4. When the attacker is going to be liquidated, frontrun txn by withdrawing assets from market A using the second account. Liquidation will fail because of the lack of liquidity.

Note that the attacker can withdraw the maximum liquidity allowed so that the liquidator cannot even liquidate partially. Also, the liquidator cannot seize from another market because the attacker only has collateral at market A. The cost of the attack is low because there's no fee on deposits or withdrawals of collateral, the only cost is the gas spent.

As you can see, the impact is high (liquidation constantly reverts) and the likelihood is also high (the attacker can execute the attack as long as necessary). It perfectly fits in the high severity category according to the Sherlock rules:

Definite loss of funds without (extensive) limitations of external conditions.

For the attacker to execute this attack, they would need to deposit the majority of the liquidity in the pool in step 2. This is not a practical attack vector.

When the attacker withdraws their assets, the utilization ratio (UR) becomes high, indicating that the attacker deposited enough liquidity to control the UR of the pool. Therefore, the likelihood of this attack is still low, and its impact is medium.

Should the attackers do these operations in order to just avoid upcoming liquidation?

While your suggested fixes provide some benefits to the protocol, this doesn't mean the issues should be considered high priority. Even low-priority issues require fixes.

[MehdiKarimi81]

According to sherlock docs: DoS has two separate scores on which it can become an issue:

The issue causes locking of funds for users for more than a week. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity.

Since only second condition is met for this issue it can be considered medium

also consider cost of attack

[santipu03]

For the attacker to execute this attack, they would need to deposit the majority of the liquidity in the pool in step 2. This is not a practical attack vector.

That's incorrect, in a highly utilized pool (e.g. 90% utilization), the attacker only needs to own a little bit more than 10% of the total liquidity to execute this attack safely. Currently, in the Exactly live contracts, there are pools with less than 1M total deposited assets, the attacker would just have to own up to 100k of liquidity there to safely execute the attack. The attack can be executed in all pools, but it will need fewer assets when attacking pools with relatively low liquidity and high utilization rates.

As I described before, the attacker can use this vulnerability to make risk-free trades limiting his collateral to a small amount of assets. If prices go up and the attacker makes a profit, there's no need to execute any attack. If prices go down and the attacker can be liquidated, he can make liquidations revert until the prices go up again or until bad debt is created, thus making the rest of the market pay for the losses of that trade.

Given that the attack doesn't actually need any external conditions to be profitable, I believe this issue warrants the high severity.

[cvetanovv]

I think this issue can remain High severity because it fits the definition given in the documentation:

Definite loss of funds without (extensive) limitations of external conditions.

The attack is straightforward to execute. A malicious user can easily front-run a liquidation transaction to prevent their own liquidation, and the cost of performing this attack is minimal.

This vulnerability breaks a core contract functionality—liquidation. It allows a malicious user to exploit the system and gain unfair advantages over other users without the risk of being liquidated.

Planning to reject the escalation and leave the issue as is.

[etherSky111]

What is a definite loss of funds?

I show an example. A attacker deposit some liquidity into the DAI pool and borrow USDC in other pool. In order to prevent future liquidation, this attacker deposit other liquidity(no need to be huge as you said) into the DAI pool using other account. When the price of DAI falls and his position becomes liquidatable, he immediatly withdraw his liquidity and increase UR of DAI pool. So his position can't be liquidatable. At this point, his debt is e.x. 1000$ and his collateral is about 1050$. But he will say: "I am happy to prevent the liquidation of my position finally."

Is this a high risk?

And also the duplication of this issue was reported as medium and the lead judge still insist this as high. There are many other issues which was downgraded as medium even though the reporters indeed thought as a high severity. However didn't escalate to avoid complex judge process.

Please check this again.

The pool with high UR (90%) means that this pool is active pool and there are many depositors and liquidity in most cases. In order to prevent any small liquidation, the attacker needs large amount of liquidity in this kind of pool.

I don't think the cost of this attack is minimal.

What is the real benefit for the attacker?

In order to just prevent liquidation, he will need pool with high UR and some liquidity and complex calculation regarding the correct deposit and withdraw amount to make the UR as the limited value of the pool.

[santichez]

We try not to interfere in the categorization of the severity of the findings, but in my opinion this should not be high at all.

[santichez]

@0x73696d616f #118 was way more valid than this one.

[cvetanovv]

I'll agree with @etherSky111 arguments and opinions on the protocol.

The attack must happen in an active pool with many depositors and liquidity. This means that many funds are needed, making it unlikely.

I also didn't see much benefit to the attacker for this issue to be of High severity. Even the duplicate that has the same root cause is submitted as Medium.

This issue breaks a core contract functionality—liquidation, but this meets the Medium severity definition.

Planning to accept the escalation and make this issue a Medium severity.

[0x73696d616f]

@cvetanovv the dup #118 is not a dup as the root cause is different. #118 is transferFrom order, this one is protocol liquidity checks. Only the impact is the same. Since this one was escalated this may still be fixed.

[cvetanovv]

The root cause is that liquidations won't work when the market is highly utilized, so #118 and #70 will be duplicates.

[0x73696d616f]

@cvetanovv fixing #70 does not fix #118 and fixing #118 does not fix #70. They are different issues that lead to the same impact.

[0x73696d616f]

Root cause of #70:

if (floatingBackupBorrowed + floatingDebt > newFloatingAssets) revert InsufficientProtocolLiquidity();

Root cause of #118:

asset.safeTransferFrom(msg.sender, address(this), repaidAssets + lendersAssets); after transferring the collateral to the liquidator, instead of before.

[0x73696d616f]

Here is one example of the issues not being dups

The market has 1000 debt 1200 collateral 200 funds are in the market (1000 are borrowed) Now let's say there is a liquidation and 1000 debt is being repaid for 1200 collateral, in the same market.

118

Debt is repaid first, and is now 0, but funds have not been transferred yet to the market (it only calls safeTransferFrom() at the end). Collateral is seized in the same market, a few lines below the debt repayment in liquidate(). In internalSeize(), beforeWithdraw() is called. In beforeWithdraw(), there is at the moment 0 debt and 1200 collateral, so the check does not revert (and the utilization, which is debt / collateral is 0, not high as the initial reason for dupping mentioned). Later, back in internalSeize(), it tries to send 1200 to the liquidator, but it reverts, as it still only has 200 funds as the funds have not yet been pulled.

Thus, this is not due to the liquidity check, but the wrong tracking of funds due to not having pulling them when the debt has already been reduced to 0. When it transfers the assets to the liquidator, debt is 0, collateral is 1200, but the market only has 200 funds, which is incorrect.

70 works just fine here

Repay debt -> 0 debt Collateral is 1200 still Utilization ratio is 0 (debt/collateral) when the check happens, so there is 0 debt and 1200 collateral and it does not revert. Non issue.

[cvetanovv]

This comment showed how the utilization ratio could be 0, and we still have an issue аnd how the root cause of the issue in #118 is the wrong tracking of funds.

That's why #118 and #70 can be split.

[MehdiKarimi81]

According to Sherlock docs: DoS has two separate scores on which it can become an issue:

The issue causes the locking of funds for users for more than a week. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity.

Since only the second condition is met for this issue it can be considered medium

also consider cost of attack

@cvetanovv What do you think about this? it is a time-sensitive function but doesn't meet the first rule ( loss of funds ), so it seems to be medium

[cvetanovv]

@MehdiKarimi81 Yes, in that comment, I wrote that I would accept the escalation, and both issues would be Medium.

[cvetanovv]

For more clarity, the decision is to accept the escalation and downgrade the severity to Medium.

70 and 118 will be downgraded to Medium severity but will be split as separate issues.

[Evert0x]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• MehdiKarimi81: accepted