Submitted by carrotsmuggler, also found by carrotsmuggler (1, 2), erebus, QiuhaoLi, Aymen0909, zhaojie, oakcobalt (1, 2), emerald7017, DadeKuma, Franfran, J4X (1, 2, 3), 3docSec, and ZanyBonzy
The liquidity removal function in the omnipool pallet lacks slippage control. There is no minimum_amount_out parameter to ensure that the user gets out at least a certain amount of tokens. This can lead to slippage losses for liquidity providers if malicious users frontrun the liquidity withdrawer.
During liquidity removal, since there are lots of different fees involved, the scenario gets complicated and a POC is used to study the effect further. A POC is presented in the next section, which has ALICE depositing LP of token_1000 to the pool, the actor LP3 carrying out a swap, and then ALICE removing liquidity immediately after. In case ALICE receives any LRNA tokens, she swaps them out to token_1000. We compare the amount of token_1000 ALICE would end up with in different scenarios.
In all scenarios, ALICE is assumed to remove liquidity at the same price she put in. However the bad actor LP3 frontruns her removal, and we want to study the effect of her losses. In scenario 1, there is no action by LP3, and ALICE deposits and withdraws, to get a baseline measurement.
Scenario 1 - Liq Add - Liq remove:
Here, there is no frontrunner in order to get a baseline measurement:
We can see ALICE started with 5000*ONE token_1000s, and ends up with 4990*ONE token_1000s. This is due to withdrawal fees, and is the acceptable baseline. Any lower amounts due to frontrunning would be unacceptable. This is a 0.1% loss.
Scenario 2 - Liq Add - token->DAI swap - Liq remove:
Here, the frontrunner devalues token_1000 by selling a bunch of it for DAI. Since the price is now lower, some of Alices shares will be burnt:
In this scenario, ALICE ends up with 4961.89*ONE token_1000s. This is nearly a 1% loss. Since some of her share tokens are burnt, the other liquidity providers profit from this, since their liquidity positions are now worth more.
Scenario 3 - Liq Add - DAI->token swap - Liq remove:
Here, the frontrunner buys up token_1000 increasing its price. Alice gets minted LRNA tokens to compensate the increase in price, but she swaps them out to token_1000 immediately. We then check her token_1000 balance and compare it to the beginning:
Here ALICE ends up with 4958.57*ONE token_1000s. This is again a 1% loss. The frontrunner can even sandwich the LRNA->token_1000 swap and even profit in this scenario.
Thus in all frontrunning scenarios, ALICE realizes a slippage loss due to insufficient parameters. The losses will be capped to 2%, since the ensure_price check in the remove_liquidity function checks if the price of the asset has not changed by more than 1% from the oracle price. Thus, the maximum price deviation that can happen is 2% (if the spot price was changed from +1% to -1%). 2% slippage is already unacceptable for a number of cases, since the industry standard for swaps has been 0.5%, and even lower for liquidity removals.
The following test was run to generate the figures above. For the different scenarios, the buy was commented (scenario 1), used to swap token->DAI (scenario 2), and used to swap DAI->token (scenario 3).
Substrate
Add a slippage limit for liquidity removal. The in-built limit of 2% is too large for most use cases.
MEV
enthusiastmartin (HydraDX) confirmed, but disagreed with severity via duplicate issue #158
Lambda (judge) commented via duplicate issue #158:
