search

sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

xiaoming90 - Malicious users can position themselves ahead of liquidation to profit from the price increase #176

Closed sherlock-admin2 closed 6 months ago

sherlock-admin2 commented 7 months ago

xiaoming90

high

Malicious users can position themselves ahead of liquidation to profit from the price increase

Summary

Malicious users can position themselves ahead of liquidation to profit from the price increase. As a result, malicious users could siphon the gains of the benign LPs, leading to the loss of assets for the victims.

Vulnerability Detail

Liquidation of large positions creates an opportunity for LPs to position themselves ahead of a liquidation and potentially profit from an increase in price (stableCollateralPerShare)

Assuming that there is a large position called $P_x$ that is on the verge of being liquidated. If the $P_x$​ gets liquidated, it is expected that a significant amount of remaining margins within this position go to the LPs, as shown in Line 130 below

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LiquidationModule.sol#L85

File: LiquidationModule.sol
085:     function liquidate(uint256 tokenId) public nonReentrant whenNotPaused liquidationInvariantChecks(vault, tokenId) {
..SNIP..
130:             // Adjust the stable collateral total to account for user's remaining margin.
131:             // If the remaining margin is greater than 0, this goes to the LPs.
132:             // Note that {`remainingMargin` - `profitLoss`} is the same as {`marginDeposited` + `accruedFunding`}.
133:             vault.updateStableCollateralTotal(int256(remainingMargin) - positionSummary.profitLoss);

When the remaining margin goes to the LPs, the stableCollateralPerShare will increase.

Bob (a malicious user) could announce a deposit order with a large amount of rETH. To prevent other keepers from executing his order, he intentionally set the keeper fee to zero so that no one would be incentivized to execute it.

In the test scripts, the minExecutabilityAge is set to 10 seconds, and maxExecutabilityAge is set to 1 minute. Thus, Bob's order will be valid for execution for 1 minute (30 blocks) after the initial 10-second (5 blocks) wait, within $T1$ and $T2$​ as shown below.

When $P_x$ is liquidated at any point of time within $T1$ and $T2$​, Bob will front-run the liquidation TX, and position his deposit order execution TX in front of the liquidation TX. Let's assume that Bob's deposit will result in Bob owning half of the shares in the vault. As a result, 50% of the gain (from the remaining margin) transferred to LPs after the liquidation will belong to Bob.

This is a zero-sum game. If Bob has not carried out this transaction, all the gain will go to the LPs. In this case, due to the planned timing of Bob's transactions, he managed to obtain most of the gains here. Bob can proceed to withdraw his shares/UNITs to realize his profit. As long as the gain is larger than the withdrawal fee, this is profitable for Bob. Bob should have done some math beforehand to ensure that he only targets the liquidation of positions that are profitable after accounting for the withdrawal fee.

image

If the liquidation of $P_x$ does not occur within $T1$ and $T2$​​, Bob could simply cancel the order after it expires, and he will get back the full deposit amount without incurring any fee. Gas fees are extremely cheap on L2 (Base), where FlatCoin is deployed on. Thus, gas cost would not have a significant impact on the profitability of this attack.

If there are a number of such positions on the verge of being liquidated, he could repeatedly announce and cancel his deposit order for an extended amount of time to wait for the opportunity to front-run them.

Impact

Malicious users could siphon the gains of the benign LPs, leading to the loss of assets for the victims.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LiquidationModule.sol#L85

Tool used

Manual Review

Recommendation

To prevent the opportunistic ordering of transactions surrounding liquidations such as the one described in this report, SNX adopts the following solutions:

sherlock-admin commented 6 months ago

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

ubl4nk commented:

invalid -> user can set keeper-fee to zero as you mentioned

takarez commented:

invalid: he can't set the keeper fee to zero as there is a check for that in _prepareAnnouncementOrder()

rashtrakoff commented 6 months ago

Bob (a malicious user) could announce a deposit order with a large amount of rETH. To prevent other keepers from executing his order, he intentionally set the keeper fee to zero so that no one would be incentivized to execute it.

I think this is incorrect. An order needs at least a minKeeperFee amount set. Otherwise, this line would fail. I believe this alone should make this issue invalid as there is a good enough incentive to execute an order.

But let's suppose it was possible to set keeperFee as 0 effectively removing any incentive. This would mean that the only person having any incentive to execute an order is the trader himself. So they will be monitoring the liquidation tx before executing their order. I would argue there is no solution here. The flagging method doesn't work in this case as it only maybe delay the front-running opportunity to when the actual liquidation tx is sent (spontaneous or forced). Maybe a PoC to demonstrate the solution would convince us of this issue.

itsermin commented 6 months ago

Agree with @rashtrakoff above. The malicious depositor cannot announce an order with 0 keeper fee. On top of that, if the depositor was to withdraw to profit immediately after, there is a withdrawal fee (likely set to 0.3%). This would further disincentivise this action.

nevillehuang commented 6 months ago

Agree with sponsors, additionally, front-running is a non-issue on base chain, see comments in #49