sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

panprog - It is possible to frontrun liquidations with self liquidation with high strain value to clear warning and keep unhealthy positions from liquidation #29

Open sherlock-admin2 opened 1 year ago

sherlock-admin2 commented 1 year ago

panprog

high

It is possible to frontrun liquidations with self liquidation with high strain value to clear warning and keep unhealthy positions from liquidation

Summary

Account liquidation involving asset swaps requires warning the account first via warn. Liquidation can only happen LIQUIDATION_GRACE_PERIOD (2 minutes) after the warning. The problem is that any liquidation clears the warning state, including partial liquidations even with very high strain value. This makes it possible to frontrun any liquidation (or just submit transactions as soon as LIQUIDATION_GRACE_PERIOD expires) by self-liquidating with very high strain amount (which basically keeps position unchanged and still unhealthy). This clears the warning state and allows account to be unliquidatable for 2 more minutes, basically preventing (DOS'ing) liquidators from performing their job.

Malicious user can open a huge borrow position with minimum margin and can keep frontrunning liquidations this way, basically allowing unhealthy position remain active forever. This can easily lead to position going into bad debt and causing loss of funds for the other protocol users (as they will not be able to withdraw all their funds due to account's bad debt).

Vulnerability Detail

Borrower.warn sets the time when the liquidation (involving swap) can happen:

slot0 = slot0_ | ((block.timestamp + LIQUIDATION_GRACE_PERIOD) << 208);

But Borrower.liquidation clears the warning regardless of whether account is healthy or not after the repayment:

_repay(repayable0, repayable1);
slot0 = (slot0_ & SLOT0_MASK_POSITIONS) | SLOT0_DIRT;

Impact

Very important protocol function (liquidation) can be DOS'ed and make the unhealthy accounts avoid liquidations for a very long time. Malicious users can thus open huge risky positions which will then go into bad debt causing loss of funds for all protocol users as they will not be able to withdraw their funds and can cause a bank run - first users will be able to withdraw, but later users won't be able to withdraw as protocol won't have enough funds for this.

Proof of concept

The scenario above is demonstrated in the test, add this to Liquidator.t.sol:

function test_liquidationFrontrun() public {
    uint256 margin0 = 1595e18;
    uint256 margin1 = 0;
    uint256 borrows0 = 0;
    uint256 borrows1 = 1e18 * 100;

    // Extra due to rounding up in liabilities
    margin0 += 1;

    deal(address(asset0), address(account), margin0);
    deal(address(asset1), address(account), margin1);

    bytes memory data = abi.encode(Action.BORROW, borrows0, borrows1);
    account.modify(this, data, (1 << 32));

    assertEq(lender0.borrowBalance(address(account)), borrows0);
    assertEq(lender1.borrowBalance(address(account)), borrows1);
    assertEq(asset0.balanceOf(address(account)), borrows0 + margin0);
    assertEq(asset1.balanceOf(address(account)), borrows1 + margin1);

    _setInterest(lender0, 10100);
    _setInterest(lender1, 10100);

    account.warn((1 << 32));

    uint40 unleashLiquidationTime = uint40((account.slot0() >> 208) % (1 << 40));
    assertEq(unleashLiquidationTime, block.timestamp + LIQUIDATION_GRACE_PERIOD);

    skip(LIQUIDATION_GRACE_PERIOD + 1);

    // listen for liquidation, or be the 1st in the block when warning is cleared
    // liquidate with very high strain, basically keeping the position, but clearing the warning
    account.liquidate(this, bytes(""), 1e10, (1 << 32));

    unleashLiquidationTime = uint40((account.slot0() >> 208) % (1 << 40));
    assertEq(unleashLiquidationTime, 0);

    // the liquidation command we've frontrun will now revert (due to warning not set: "Aloe: grace")
    vm.expectRevert();
    account.liquidate(this, bytes(""), 1, (1 << 32));
}

Code Snippet

Borrower.warn sets the liquidation timer: https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Borrower.sol#L171

Borrower.liquidate clears it regardless of strain: https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Borrower.sol#L281

This makes any liquidation (even the one which doesn't affect assets much due to high strain amount) clear the warning.

Tool used

Manual Review

Recommendation

Consider clearing "warn" status only if account is healthy after liquidation.

panprog commented 1 year ago

Escalate

This should be High, because the issue allows to freely (other than gas) DOS your own liquidation while keeping your account unhealthy. This opens up a lot of attack vectors and can substantially harm the protocol by creating bad debt.

In previous contests DOS'ing your liquidation was considered High.

Additionally, the user doesn't need to do this every transaction, just every 2 minutes to clear warn, so this is very cheap, basically free compared to possible profit user can get (for example, if he has 2 reverse "positions" opened - borrow usdt against eth and borrow eth against usdt - one of them goes into bad debt he doesn't have to pay, the other will be in a profit) and possible protocol loss of funds (due to bad debt).

Considering all this, I believe this is clearly a high issue, not medium.

sherlock-admin2 commented 1 year ago

Escalate

This should be High, because the issue allows to freely (other than gas) DOS your own liquidation while keeping your account unhealthy. This opens up a lot of attack vectors and can substantially harm the protocol by creating bad debt.

In previous contests DOS'ing your liquidation was considered High.

Additionally, the user doesn't need to do this every transaction, just every 2 minutes to clear warn, so this is very cheap, basically free compared to possible profit user can get (for example, if he has 2 reverse "positions" opened - borrow usdt against eth and borrow eth against usdt - one of them goes into bad debt he doesn't have to pay, the other will be in a profit) and possible protocol loss of funds (due to bad debt).

Considering all this, I believe this is clearly a high issue, not 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.

roguereddwarf commented 1 year ago

@panprog This finding requires the assumption that you can over a long period of time continuously front-run liquidation bots. You need to do it every 2 minutes.

Even if there's a small chance of 1% that the liquidation bot is faster, the chance that this DOS stays active for more than 24 hours is: 24 hours / 2 minutes = 720. Chance of 720 successful front-runs in a row: 0.99^720 = ~0.07%.

Hence this is not a High due to Low likelihood. Medium is justified.

panprog commented 1 year ago

In the previous contests front-running liquidator to prevent liquidation was considered high, even when it was required to front-run liquidator every block, for example in Symmetrical: https://github.com/sherlock-audit/2023-06-symmetrical-judging/issues/233

So for judging consistency I believe that similar issues should be judged high as well. especially since this issue requires front-running only once per 2 minutes, not once per block.

roguereddwarf commented 1 year ago

@panprog

Thanks for bringing this other issue to my attention I hadn't seen that. In that case I can see where the "High" severity is coming from.

Just want to add that due to the implied volatility calculations in Aloe there is a margin of safety built into Aloe and liquidations don't need to occur instantly, liquidations can take up to 24 hours and there should be no bad debt. I.e. the price should not be able to move within 24 hours in a way that would cause bad debt.

And based on my previous calculation, the risk of bad debt is therefore Low.

I still think this should be Medium based on the protocol-specific considerations.

panprog commented 1 year ago

@roguereddwarf I certainly understand your reasoning and it's valid. However, while this protocol has much larger safety margin, which requires longer time until bad debt can happen, this is still comparable to my reference in Symmetrical, where frontrunning liquidation required doing this every block, but bad debt could have happened sooner than here. So, number of blocks in Symmetrical and number of 2-minute intervals here are probably roughly similar.

I want to add another reason for this to be high - in the current state of the code there are the other issues (such as volatility manipulations) which can be combined with this one to make it high.

Trumpero commented 1 year ago

Planning to accept escalation and upgrade this issue to high.

The impact should be high because the attacker can prevent liquidation by front-running. The likelihood should be between medium and high, even if it requires many successful instances of front-running.

Czar102 commented 12 months ago

Result: High Has duplicates

sherlock-admin2 commented 12 months ago

Escalations have been resolved successfully!

Escalation status:

haydenshively commented 11 months ago

Fixed in https://github.com/aloelabs/aloe-ii/pull/223

roguereddwarf commented 11 months ago

Mitigation Review:

The auction timestamp is only reset when either the Borrower is healthy after the liquidation or the Borrower is insolvent and bad debt is erased. In both cases, the Borrower ends up in a state where it's no longer up for liquidation. The issue is therefore fixed.