HSP - Leverage trader can front-run liquidation to close position by placing limit order in advance

[sherlock-admin2]

HSP

high

Leverage trader can front-run liquidation to close position by placing limit order in advance

Summary

Leverage trader can front-run liquidation to close position by placing limit order in advance.

Vulnerability Detail

When a leverage position is opened by a leverage trader, this position can be liquidated if the collateral price drops to liquidation price.

Liquidator calls liquidate(...) function to liquidate, this function can be executed without any delay.

At the same time, a leverage position can also be closed by the trader even if the collateral price is below the liquidation price.

To close a position, leverage trader should first create a close order by making a announcement, only if the minimum time delay is reached the order can be executed.

With such limitation, it's unlikely leverage trader can close a liquidatable position before it's liquidated by a liquidator. However, this is not always true, the trader can actually front-run liquidation to close position by placing limit order in advance.

A limit order can only be executed if the collateral price is below priceUpperThreshold or the price is above priceUpperThreshold:

        if (price <= _limitOrder.priceLowerThreshold) {
            minFillPrice = 0; // can execute below lower limit price threshold
        } else if (price >= _limitOrder.priceUpperThreshold) {
            minFillPrice = _limitOrder.priceUpperThreshold;
        } else {
            revert FlatcoinErrors.LimitOrderPriceNotInRange(
                price,
                _limitOrder.priceLowerThreshold,
                _limitOrder.priceUpperThreshold
            );
        }

So a leverage trader can create a limit order right after opening a position, and set priceLowerThreshold to liquidation price and priceUpperThreshold to type(uint256).max, thus the position can only be closed when the collateral price is no larger than liquidation price. If collateral price drops to liquidation price, the trader can front-run any liquidator to execute the limit order to close the position, as a result, the trader "rescues" some funds whereas liquidator gets nothing but a reverted transaction.

Please see the test codes:

    function test_audit_frontrun_liquidation_and_close_position() public {
        uint256 oraclePrice = 1000e8;

        announceAndExecuteDeposit({
            traderAccount: alice, 
            keeperAccount: keeper, 
            depositAmount: 100e18, 
            oraclePrice: oraclePrice, 
            keeperFeeAmount: 0
        });

        // Bob opens a position
        uint256 tokenId = announceAndExecuteLeverageOpen({
            traderAccount: bob,
            keeperAccount: keeper,
            margin: 50e18,
            additionalSize: 100e18,
            oraclePrice: oraclePrice,
            keeperFeeAmount: 0
        });

        // Bob place a limit order, priceLowerThreshold is liqPrice and priceUpperThreshold is type(uint256).max
        vm.startPrank(bob);
        uint256 liqPrice = liquidationModProxy.liquidationPrice(tokenId);
        limitOrderProxy.announceLimitOrder({
            tokenId: tokenId,
            priceLowerThreshold: liqPrice,
            priceUpperThreshold: type(uint256).max
        });
        vm.stopPrank();

        skip(uint256(vaultProxy.minExecutabilityAge()));

        // Keeper cannot execute the limit order because price is not in range
        vm.startPrank(keeper);
        bytes[] memory priceUpdateData = getPriceUpdateData(oraclePrice);
        vm.expectRevert(abi.encodeWithSelector(FlatcoinErrors.LimitOrderPriceNotInRange.selector, 1000e18, liqPrice, type(uint256).max));
        limitOrderProxy.executeLimitOrder{value: 1}(tokenId, priceUpdateData);
        vm.stopPrank();

        uint256 bobCollateralBalanceAfterOpenningPosition = WETH.balanceOf(bob);

        // Price drops
        skip(2 days);
        uint256 liquidationPrice = (liqPrice - 1e18) / 1e10;
        setWethPrice(liquidationPrice);

        // Bob's position can be liquidated
        assertTrue(liquidationModProxy.canLiquidate(tokenId));

        // Bob frontruns and closes his position
        vm.startPrank(bob);
        priceUpdateData = getPriceUpdateData(oraclePrice);
        limitOrderProxy.executeLimitOrder{value: 1}(tokenId, priceUpdateData);
        vm.stopPrank();

        uint256 bobCollateralBalanceAfterClosingPosition = WETH.balanceOf(bob);

        // Bob get some collateral back
        assertTrue(bobCollateralBalanceAfterClosingPosition - bobCollateralBalanceAfterOpenningPosition > 0);

        skip(1);

        // Liquidator cannot liquidate because postion has been closed
        vm.startPrank(liquidator);
        priceUpdateData = getPriceUpdateData(oraclePrice);
        vm.expectRevert(abi.encodeWithSelector(FlatcoinErrors.CannotLiquidate.selector, tokenId));
        liquidationModProxy.liquidate{value: 1}(tokenId, priceUpdateData);
        vm.stopPrank();
    }

Impact

Leverage trader cannot be liquidated.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LeverageModule.sol#L255-L259

Tool used

Manual Review

Recommendation

Should not allow leverage trader to close a position if the collateral price drops to liquidation price.

    function executeClose(
        address _account,
        address _keeper,
        FlatcoinStructs.Order calldata _order
    ) external whenNotPaused onlyAuthorizedModule returns (int256 settledMargin) {
        ...

+       if (
+           ILiquidationModule(vault.moduleAddress(FlatcoinModuleKeys._LIQUIDATION_MODULE_KEY)).canLiquidate(
+               announcedClose.tokenId
+           )
+       ) revert FlatcoinErrors.PositionCreatesBadDebt();

        ...
    }

[nevillehuang]

Request PoC to facilitate discussion between sponsor and watson

[sherlock-admin2]

PoC requested from @0xhsp

Requests remaining: 13

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: attacker can avoid liquidation: high(9)

[0xhsp]

For POC please see the test codes in the report.

The main point is that attacker places a limit order, priceLowerThreshold is liqPrice and priceUpperThreshold is type(uint256).max, so this order can only be executed when the position can be liquidated.

However, when any liquidator tries to liquidate, attacker can simply front-run and close the position, hence avoid liquidation.

[rashtrakoff]

It's important to understand the intent behind liquidations. Liquidations need to happen at the earliest to close a position which can create a potential bad debt. Now, it doesn't matter who closes it as long as someone closes the position. When someone says that the position "cannot be liquidated" it will be a valid statement in case no one can close a position which is supposed to be closed. In this case, however, the trader is closing the position thus removing the potential bad debt-causing position.

We could add a check not to let a liquidatable position be closed by the trader. Still, it doesn't serve any real purpose apart from completely locking out the trader (owner of the position) from modifying their position.

Now, coming to the front-running issue, this is unavoidable. Anyone can front-run a transaction (provided there is a public mempool and the sequencer is not ordering transactions on a first-come-first/highest-gas-first basis). Let's take an example; a liquidator can be front-run by another liquidator trying to close the bad position and they can engage in a gas escalation battle to determine the ordering or the trader attempts to modify their position by increasing margin or decreasing size thus avoiding liquidation. Ultimately liquidators will need to use whatever tactics are available to them (private mempools, flashbots-like services etc) to make liquidations profitable for themselves.

All this being said, if you can provide a PoC for bricking liquidations of certain positions due to front-running, that's something we can consider.

[0xhsp]

It's important to understand the intent behind liquidations. Liquidations need to happen at the earliest to close a position which can create a potential bad debt. Now, it doesn't matter who closes it as long as someone closes the position. When someone says that the position "cannot be liquidated" it will be a valid statement in case no one can close a position which is supposed to be closed. In this case, however, the trader is closing the position thus removing the potential bad debt-causing position.

We could add a check not to let a liquidatable position be closed by the trader. Still, it doesn't serve any real purpose apart from completely locking out the trader (owner of the position) from modifying their position.

Now, coming to the front-running issue, this is unavoidable. Anyone can front-run a transaction (provided there is a public mempool and the sequencer is not ordering transactions on a first-come-first/highest-gas-first basis). Let's take an example; a liquidator can be front-run by another liquidator trying to close the bad position and they can engage in a gas escalation battle to determine the ordering or the trader attempts to modify their position by increasing margin or decreasing size thus avoiding liquidation. Ultimately liquidators will need to use whatever tactics are available to them (private mempools, flashbots-like services etc) to make liquidations profitable for themselves.

All this being said, if you can provide a PoC for bricking liquidations of certain positions due to front-running, that's something we can consider.

It does matter who closes the position. If the position is closed by a liquidator, the liquidator gets liquidation fee as bonus and LP holders get the remaining margin, however if the position is closed by the trader who owns the position, settled margin will be transferred to the trader and LP holders get nothing (except the fee which is much less than the margin), this largely reduces the profits that should be earned by the LP holders and may also discourage liquidators to liquidate the positions, this is no good the protocol.

Not allowing a liquidatable position be closed by the trader does not mean a completely locking, the trader still can add more margins to the position based on the current implementation, there is a check not allowing trader to decrease margin if the position would be liquidatable after adjustment, this actually serves the same purpose as adding a check not to let a liquidatable position be closed by the trader.

Coming to the front-running issue, as said above, it's ok to let liquidator be front-run by another liquidator because the protocol gets the benefits, but a liquidator be front-run by the trader is quite another matter, and liquidator has no chance to win the gas escalation battle. Liquidators are incentivized by liquidation fee, so a liquidator will not set the gas price higher than the liquidation fee, however, the trader can set a gas price slightly higher than liquidation fee and successfully front-run. Take the example in the report, the settled margin at liquidation price is 0.6 eth, assume liquidation fee ratio is 0.5%, liquidation fee is 0.149 ether, so even if paid high gas fee, the trader can still receive 0.45 eth, and the trader's gain is the LP holders' loss.

Private mempools, flashbots-like services do not affect the validity of this issue, the trader does not need know to when the liquidation happens, he simply executes the limit order right after price drops, just like the liquidators, but the trader will eventually win because he is the only one who offer gas price higher than liquiation fee.

[nevillehuang]

@0xhsp See comments in #49, front-running is not possible on base chain since users interact with the protocol via base chain directly, so I believe this issue is possibly invalid. Your original submission is only showing a front-run scenario.

[0xhsp]

@nevillehuang By front-run I mean paying more priority fee to get one transaction executed earlier than others, and I didn't mention anything about mempool.

Normally, a public mempool is required as the attacker needs to know WHEN one specific transaction is submitted, however, in the context of this issue, the trader does not need to know when the liquidation happen because he will try to close position as soon as price drops, it's possible some liquidators try to liquidate at the same time too, but as I explained in my earlier comment, the trader will win and successfully front-run.

I am sorry for any confusion but I believe this issue is still valid.

[nevillehuang]

@0xhsp To front-run, you need a public mem-pool so I don't get what you are saying with respect to your original submission which only presents a front-running scenario, not your additional comments made. I am inclined to close this issue for now, will discuss further with sponsor, so if I do, feel free to escalate if you think otherwise.

[0xhsp]

Escalate.

This issue is valid high.

Just because there is no public mempool does not mean trader cannot front-run.

As stated in the report:

If collateral price drops to liquidation price, the trader can front-run any liquidator to execute the limit order to close the position, as a result, the trader "rescues" some funds whereas liquidator gets nothing but a reverted transaction.

This applies to the following scenario:

1. Trader announces a limit order for a position he just opened;
2. The position is monitored by the trader and a liquidator;
3. When price drops and the position is liquidatable, trader submits transaction to execute the limit order (minExecutabilityAge has passed) and the liquidator tries to liquidate;
4. Trader is willing to pay more priority fee than liquidation fee whereas the liquidator isn't, so trader's transaction is executed first and the liquidator's transaction is front-run.

As we can see from above, no mempool is utilized yet the liquidator is still front-run. As a result, liquidator won't be able to liquidate any positions which are liquidatable.

[sherlock-admin2]

Escalate.

This issue is valid high.

Just because there is no public mempool does not mean trader cannot front-run.

As stated in the report:

If collateral price drops to liquidation price, the trader can front-run any liquidator to execute the limit order to close the position, as a result, the trader "rescues" some funds whereas liquidator gets nothing but a reverted transaction.

This applies to the following scenario:

1. Trader announces a limit order for a position he just opened;
2. The position is monitored by the trader and a liquidator;
3. When price drops and the position is liquidatable, trader submits transaction to execute the limit order (minExecutabilityAge has passed) and the liquidator tries to liquidate;
4. Trader is willing to pay more priority fee than liquidation fee whereas the liquidator isn't, so trader's transaction is executed first and the liquidator's transaction is front-run.

As we can see from above, no mempool is utilized yet the liquidator is still front-run. As a result, liquidator won't be able to liquidate any positions which are liquidatable.

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.

[Evert0x]

I don't think the escalation describes a scenario that is highlighting an issue. It's describing how the protocol is supposed to function.

In case the price drops to liquidation price, the liquidator is able to liquidate the trader, That's the deign of the protocol.

Planning to reject escalation and keep issue state as is

[0xhsp]

@Evert0x Please reconsider this issue.

This issue was originally invalidated for that trader cannot front-run, so the escalation is only to explain how front-run is feasible.

In case the price drops to liquidation price, the liquidator is able to liquidate the trader, That's the deign of the protocol.

The issue essentially describes an vulnerability that the liquidator is not able to liquidate the trader.

Please see my earlier comment here for why should not allow trader to close a liquidatable position.

To better understand this, let's see how liquidation works in AAVE.

In AAVE, DAI's Loan to Value is 77% and Liquidation Threshold is 80%, this means to borrow 770 DAI, user need to deposit 1000 DAI worth of the collateral, when collateral value drops below 962.5u (770 / 80%), the position can be liquidated. The only way for user to avoid liquidation is to deposit more collateral and if user fails to do so, the collateral is seized as penalty (in the case of the example the trader lose 192.5u).

However in Flatmoney, the penalty is not guaranteed as trader can avoid the liquidation by simply closing the underwater position (as described in the report, the trader can always win the gas war), trader pays nothing and loses nothing whereas borrower suffers the risk of bad debt but receive no compensation.

[Evert0x]

@0xhsp thanks for you detailed reply.

I understand the issue as follows:

There can be a underwater position that is able to get a) Closed by the trader b) Liquidated by anyone

Option a) would be more beneficial for the trader as he get's to keep the margin Option b) would be more beneficial for the LPs as it maximizes the profits

However, the only way a) can be realized if they front-run people that are trying to benefit of executing b). The room for opportunity is extremely small as it requires a higher priority in the same block b) is executed, which is executed as soon as the position can be liquidated.

I don't believe this report identifies an issue.

[0xhsp]

@Evert0x With all due respect, I don't think the room for opportunity is extremely small. It's only fair to consider the trader have the same ability as the liquidator when we do the judging, so as soon as position can be liquidated, trader and liquidator are likely to have their transaction in the same block, and trader can pay more priority fee higher than the liquidation fee to get the priority.

Besides, it's not just profits to the LPs, an underwater position can be viewed as an unrealized bad debt to the protocol, it needs to be offset by the settled margin when the position is liquidated, however if the position is closed by the trader (again, even if trader may not able to front-run every time, the winning chance is definitely not small), the loss is realized and suffered by the LPs.

---

Would like to add more comment on the room for opportunity, it's incorrect to say getting transactions in the same block is the only way to realize a). In fact, a) can also be realized if trader's transactions is included in the block prior to the block which includes liquidator's transaction.

Let's assume reasonably: 1) The chance of trader's transaction gets included in the block prior to the liquidator's block is 40%; 2) The chance of trader's transaction gets included in the block after the liquidator's block is 40%; 3) The chance of trader's transaction and liquidator's transaction get included in the same block is 20%.

Simply do the math we can see the chance that a) can be realized is 60%, I believe the room is big enough for the trader to cause damages to the LPs. (In fact, trader can be more efficient than liquidator as the liquidator has many more positions to monitor whereas the trader only need to monitor one)

[securitygrid]

Please notice that there are multiple keepers: from third-parties and from the protocol. So the following case will not happen.

Trader is willing to pay more priority fee than liquidation fee whereas the liquidator isn't, so trader's transaction is executed first and the liquidator's transaction is front-run.

[0xhsp]

Please notice that there are multiple keepers: from third-parties and from the protocol. So the following case will not happen.

Trader is willing to pay more priority fee than liquidation fee whereas the liquidator isn't, so trader's transaction is executed first and the liquidator's transaction is front-run.

Third-party liquidators are incentivised by liquidation fee to liquidate a position, so they will not pay more priority fee than the trader; Keepers from protocol are used to execute orders and only to liquidate positions that has very few settled margin which does not incentive the other liquidators, this is rare case because the underwater position is usually closed/liquidated at the earliest, and the bad debt has occurred and the damage has been done even if it is that case, also it's unreasonable for the keepers to pay too much to liquidate such a position, so I believe they can be ignored in the discussion.

In the real world MEV, if one arbitrager can front-run others mainly depends on how much gas price the arbitrager is willing to pay, the number of arbitragers does not really matter.

[securitygrid]

the underwater position is usually closed/liquidated at the earliest,

Why is a position liquidated only when it has bad debts? It will be liquidated by the liquidator as soon as it can be liquidated.

[0xhsp]

Depending on the actual price when the trader closes the underwater position, it's either profit loss or bad debt to the LPs.

[RealLTDingZhen]

I believe it is a design choice

[0xhsp]

I believe it is a design choice

Sponsors allow this mainly because that 'liquidation price keeps changing because of funding rates', but this does not mean the loss to LPs is acceptable. So the design is flawed and needs to be fixed (actually you may notice this issue has a 'will fix' label).

[RealLTDingZhen]

but this does not mean the loss to LPs is acceptable.

But, if the order is liquidated instead of closed, LPs would still suffer loss because bad debt is socialized😂

[0xhsp]

but this does not mean the loss to LPs is acceptable.

But, if the order is liquidated instead of closed, LPs would still suffer loss because bad debt is socialized😂

Not necessarily, it depends on the price when the position is liquidated.

There is liquidation buffer in the position, when the price is slightly below liquidation price, settled margin can be viewed as profit to LPs.

Most importantly, when bad debt is actually socialized, we definitely don't want things to be worse, we need the bad debt to be offset by the settled margin, but it surely won't work if the position is closed by trader.

[Evert0x]

With all due respect, I don't think the room for opportunity is extremely small. It's only fair to consider the trader have the same ability as the liquidator when we do the judging, so as soon as position can be liquidated, trader and liquidator are likely to have their transaction in the same block, and trader can pay more priority fee higher than the liquidation fee to get the priority.

That's why the room for opportunity is extremely small. It's only a single block.

The loss is realized and suffered by the LPs.

Protecting LPs for a price difference between two blocks (2 second block time) is not a bug in the protocol, it's a design choice.

[0xhsp]

@Evert0x If you insist on thinking the chance for transactions included in the same block is small, that's fine. What about the trader's transaction included in the block before the liquidator's? The 2 second block time actually give trader more advantage over the liquidator.

Say liquidator has 100 positions to monitor (few are liquidatable) and when he find the underwater position, the 2 second block has already passed, however the trader are able to submit close transaction for his one single position within 2 seconds and gets transaction included and executed several blocks earlier. I don't see how LPs are protected in such scenario.

[Evert0x]

What about the trader's transaction included in the block before the liquidator's?

This is when the position is not able to get liquidated, so it will not be an issue.

The illustrated scenario is not related to the on-chain contracts and incentives.

[RealLTDingZhen]

I believe sponsor's comment already confirms that this is a design choice.

It's important to understand the intent behind liquidations. Liquidations need to happen at the earliest to close a position which can create a potential bad debt. Now, it doesn't matter who closes it as long as someone closes the position. When someone says that the position "cannot be liquidated" it will be a valid statement in case no one can close a position which is supposed to be closed. In this case, however, the trader is closing the position thus removing the potential bad debt-causing position.

[Evert0x]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xhsp: rejected