Bad actor may steal deposit return when liquidating a trade

[code423n4]

Handle

jonah1005

Vulnerability details

Impact

The OpenLev contract handles liquidation this way.

1. check whether the position is dangerous.
2. taking penalty from the position and giving it to the protocol and the liquidator.
3. sell the borrowed tokens through dex.
4. return whatever is left to the position owner.

The liquidator has an opportunity to sandwich this trade and return less to the position owner.

I consider this issue is aware of the team as there's a check of maxLiquidationPriceDiffientRatio. However, this arbitrage space can be mitigated by the design, and I consider this is still a medium-risk issue.

Proof of Concept

Please refer to the liquidate function. OpenLevV1.sol#L239-L319

Also, the test shows that the protocol allows the liquidator to liquidate trades without min return. OpenLevV1UniV3Test1.js#L239-L244

Recommended Mitigation Steps

Allowing any user to initiate a trade on behalf of (other users/ protocols) is an anti-pattern. please refer to this article: https://media.dedaub.com/yield-skimming-forcing-bad-swaps-on-yield-farming-397361fd7c72

A more common liquidation design would be:

1. check whether the position is dangerous.
2. takes penalty from the position and returns it to the owner.
3. sell the borrowed tokens through dex.
4. return whatever left to the liquidator

In this case, the liquidator has no incentive to sandwich his own trade. He should protect his trade by setting a reasonable minBuy / maxSell.

[ColaM12]

Liquidator always has a chance to alter price.

[0xleastwood]

This issues seems entirely valid. Liquidators are incentivized to sandwich attack their own liquidation to minimise any leftover tokens. It definitely makes sense to have these leftover tokens distributed to the liquidator. Although I can understand why there might be no need to fix this. It can be costly for liquidators to perform such sandwich attacks, so it does seem rather uncommon.

I guess what might be good for me to understand is how might there be leftover tokens after selling borrowed tokens? Would this be the case where the borrowed tokens ends up covering the shortfall and more? @ColaM12

[ColaM12]

Liquidator always have a chance to sandwich or front run an attack. This change may fix the sandwich case, but for front run this issue remains the same. Therefore, I do not see any improvement in blocking attacks by switching orders.

[0xleastwood]

As per sponsor's comment. It sounds like the liquidation process is meant to be unbiased. Value does not seem to be extracted from the protocol in any way and it is likely it'll be costly for the liquidator to perform such an attack without also being frontrun. Considering this, I'll mark it as 1 (Low).

[0xleastwood]

IMO, the issue outlined is still valid but not exactly a worthwhile fix.