sherlock-audit / 2024-02-perpetual-judging

2 stars 2 forks source link

PUSH0 - If both makers make a loss, closing positions may be impossible, discouraging liquidations #90

Closed sherlock-admin4 closed 7 months ago

sherlock-admin4 commented 7 months ago

PUSH0

high

If both makers make a loss, closing positions may be impossible, discouraging liquidations

Summary

In the case that both makers are in a loss position, the _checkMinMarginRatio() will trigger most of the time, preventing position closes, which will discourage liquidations.

Vulnerability Detail

Consider the following scenario:

Then the short positions (especially high-leverage ones) will likely go underwater before liquidators start to act. Note that the same can be said for the other trading direction as well.

Now, there is no problem yet if no traders can realize their profits or losses due to the lack of makers. However, we present several scenarios where an already-underwater short trader can close their position and realize their loss:

The end result is that, since liquidators are not able to profitably operate, unhealthy positions cannot be liquidated, and bad debt is accrued.

Impact

If both makers are in a losing position (or any other situation that results in being unable to add positions against them), closing other positions may be impossible, discouraging liquidations.

Code Snippet

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/maker/OracleMaker.sol#L377-L382

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/maker/SpotHedgeBaseMaker.sol#L685-L689

Tool used

Manual review

Recommendation

Introduce a maker specifically for closing liquidated positions and not for any other positions, to account for the case when both makers are out of commission. They can take the exact design as one of the current makers, preferrably Oracle Maker (two Spot Hedge Makers on the same market would share the same UniV3 liquidity, which defeats the purpose).

This is known as a "backstop" pool in other protocols, notably some lending protocols.

sherlock-admin2 commented 7 months ago

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

santipu_ commented:

High - Root cause in the inability to liquidate whitelisted makers

rc56 commented 7 months ago

@lnxrp:

nevillehuang commented 7 months ago

request poc

sherlock-admin2 commented 7 months ago

PoC requested from @PUSH0

Requests remaining: 6

Oot2k commented 7 months ago

minMarginRatio preventing position reduction is a known issue

We agree. However, this issue points out a temporary DoS scenario where both makers are out of commission due to price movements. The root cause of this issue is the lack of guaranteed liquidity avenue in the case of both makers on a loss.

Unliquidatable whitelisted maker is also a known issue

We agree, however this issue does not rely on whether a whitelisted maker is liquidatable or not.

Note the two makers (OracleMaker & SpotHedgeMaker) are not expected to be the only makers in the market, there will also be ad hoc orders from off-chain market makers

This is a dangerous assumption because:

All in all we still believe this to be a high severity issue.

midori-fuse commented 7 months ago

Indeed that what we're pointing here is not an issue with whitelisted makers, but rather a temporary DoS scenario, where, if both makers are losing, then there may be no liquidity avenue to close out the liquidated positions (or any positions at all). There are only three ways for the maker's DoS to resolve from there:

None of these scenario can be considered likely, or even guaranteed, in the near future, given the history of cryptocurrency price. Generally a liquidator only wants to operate when they can guarantee a profit for themselves. Ad-hoc makers are not guaranteed liquidity (i.e. there is no guarantee that there are enough pending orders in the correct price range to close the liquidated positions on).

lnxrp commented 7 months ago
  • The two dedicated makers (Oracle and SHBM) are the only ones guaranteed to provide liquidity.
  • In low cap markets there might be no other makers when liquidation accrues

Ad-hoc makers are not guaranteed liquidity (i.e. there is no guarantee that there are enough pending orders in the correct price range to close the liquidated positions on).

I agree that guaranteed liquidity would greatly help stabilize the market in volatile times, but I'd like to dispute on the concept of using the two on-chain makers as guaranteed liquidity (or backstop-LP in CEX): there are more efficient options.

IllIllI000 commented 7 months ago

@nevillehuang The root cause of this issue is the lack of guaranteed liquidity avenue in the case of both makers on a loss nowhere does it say that there is any guarantee of liquidity, so this sounds like a feature request to make the two primary makers take on risk even when they're designed not to, when their risk parameters say they shouldn't. The sponsor and the docs say that there should be other non-automated makers, and they should handle the task of liquidation in such events.

Oot2k commented 7 months ago

escalate

I want to bring this issue up for discussion again. I strongly disagree with senior comment, this is not a feature request in any way. And the statement of maker taking addition risk is wrong.

Shortly in regards to comment from senior: In case the market moves up, maker is short and short position would get liquidated, the liquidator would have to open a long position to close his transferred short position. (In perpetual on liquidations the underwater position is transferred to the liquidator, and not closed directly) Opening this long position would actually derisk the maker because the underwater position of maker would get reduced.

But this should not be part of the discussion if this issue is valid or not, but rather if the proposed fix is good. This does not change the severity or deny the existents of this issue, rather opens a discussion on how to fix it.

Referring to sponsors and seniors comment this issue is a major problem, but the fix is non trivial.

For this reason I request a rejudgement of this issue.

sherlock-admin2 commented 7 months ago

escalate

I want to bring this issue up for discussion again. I strongly disagree with senior comment, this is not a feature request in any way. And the statement of maker taking addition risk is wrong.

Shortly in regards to comment from senior: In case the market moves up, maker is short and short position would get liquidated, the liquidator would have to open a long position to close his transferred short position. (In perpetual on liquidations the underwater position is transferred to the liquidator, and not closed directly) Opening this long position would actually derisk the maker because the underwater position of maker would get reduced.

But this should not be part of the discussion if this issue is valid or not, but rather if the proposed fix is good. This does not change the severity or deny the existents of this issue, rather opens a discussion on how to fix it.

Referring to sponsors and seniors comment this issue is a major problem, but the fix is non trivial.

For this reason I request a rejudgement of this issue.

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.

nevillehuang commented 7 months ago

@Oot2k @midori-fuse I think given the constraint highlighted by the sponsor here, it would be good to see a coded/numerical PoC on how this issue can cause issues that also bypasses the constraints mentioned.

WangSecurity commented 6 months ago

@Oot2k @midori-fuse want to loop you in here. Firstly, can you give any numerical PoC (if coded, would be ideal). Secondly, I see you say that the root cause is indeed not in whitelisted makers cannot be liquidated, but if both of them are in a losing position then the liquidations are impossible, correct? If it was possible to liquidate them, would it fix the issue? Thirdly, this sentence is quite confusing to me:

closing other positions may be impossible, discouraging liquidations.

Is it completely impossible and liquidations will revert or the liquidation will go through, but liquidators won't receive they reward now, and will receive it later?

Oot2k commented 6 months ago

Hi @WangSecurity I dont think a numerical POC will show much different then just explaining like the issue does. Liquidating makers will not fix this issue because they dont have to be in a liquidatable state.

The issue summarized:

WangSecurity commented 6 months ago

Firstly, although this situation may occur, the protocol doesn't guarantee liquidity. Moreover, according to docs, there should be other non-automated makers. Hence, I believe this report is not an issue, but a design decision.

Planning to reject the escalation and leave the issue as it is.

Evert0x commented 6 months ago

Result: Invalid Unique

sherlock-admin4 commented 6 months ago

Escalations have been resolved successfully!

Escalation status: