ge6a - Loss of funds for trader because whitelisted maker can't be liquidated

[sherlock-admin3]

ge6a

high

Loss of funds for trader because whitelisted maker can't be liquidated

Summary

In the current implementation, a whitelisted maker cannot be liquidated, thus it can accumulate losses even after the available margin is exhausted. This leads to losses for the traders because they won't be able to close their profitable positions due to a revert on _checkMarginRequirement.

Vulnerability Detail

Scenario:

1) A trader opens a long position against a whitelisted maker. 2) After some time, the price increases significantly. At this point, there are more long positions open than short positions, so this maker incurs large losses. The margin is not sufficient to cover them. However, the maker cannot be liquidated and continues to accumulate losses. 3) The trader decides to close their position and withdraw their profit. However, this cannot happen because the _closePositionFor function calls _checkMarginRequirement for the maker. vault.getFreeCollateralForTrade is < 0, leading to a revert. The trader cannot close their position. The only solution is for LPs to deposit additional collateral to cover the losses, but it doesn't make sense to deposit funds to cover losses. 4) The price decreases, and the trader loses their profit. Due to fees or a sudden price drop, the trader may also lose part of the margin.

In the described circumstances, the trader takes the risk by opening a position, but there is no way to close it and withdraw the profit. Thus, instead of gaining from the winning position, they incur losses.

POC

```solidity function testNotEnoughBalancePnlPool() public { _deposit(marketId, taker1, 10000e6); vm.prank(taker1); clearingHouse.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(maker), isBaseToQuote: false, isExactInput: false, amount: 1000 ether, oppositeAmountBound: 100000 ether, deadline: block.timestamp, makerData: "" }) ); console.log("Pnl pool balance: %d", vault.getPnlPoolBalance(marketId)); maker.setBaseToQuotePrice(111e18); maker2.setBaseToQuotePrice(111e18); _mockPythPrice(111, 0); console.log("getFreeCollateralForTrade:"); console.logInt(vault.getFreeCollateralForTrade(marketId, address(maker), 111e18, MarginRequirementType.MAINTENANCE)); vm.prank(taker1); //this will revert with NotEnoughFreeCollateral error clearingHouse.closePosition( IClearingHouse.ClosePositionParams({ marketId: marketId, maker: address(maker), oppositeAmountBound: 1000 ether, deadline: block.timestamp, makerData: "" }) ); } ```

Impact

Loss of funds for the trader + broken core functionality because of inability to close a position.

Code Snippet

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L267-L356

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L488-L524

Tool used

Manual Review

Recommendation

The best solution is to implement liquidation for whitelisted maker. If not possible, you can mitigate the issue with larger margin requirement for whitelisted makers.

[sherlock-admin4]

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

takarez commented:

the natsepc says that liquidating whitelisted maker is not allwoed, so i will considered this a know issue

[vinta]

Agree with takarez that this is a known issue.

[sherlock-admin2]

Escalate

According to the Sherlock docs, the Sherlock rules for valid issues have more weight than the code comments:

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel. While considering the validity of an issue in case of any conflict the sources of truth are prioritized in the above order.

Given that this issue is not mentioned in the contest README and clearly demonstrates that it will cause a loss of funds, I think it should be considered a valid issue, as well as its duplicates.

You've deleted an escalation for this issue.

[IllIllI000]

@santipu03 nowhere does it say that there's a guarantee that the makers will provide liquidity 100% of the time, and in any market, if there's no liquidity at the price you want, that's a you problem, not a security issue. There are expected to be other market participants who can provide liquidity for closing positions, and even if bad debt does occur, the system allows for and tracks it. Just looking at the code, not just the comments, it expects that whitelisted makers can't be liquidated, so it's not clear that anything in the design is being violated. I agree that the hierarchy is a bit confusing and I would like it to be updated and clarified, but if this escalation can be closed on facts not relating specifically to the ambiguities in the hierarchy, I think we may not get such an update.

[santipu03]

@IllIllI000 Now that I have checked better the 3 duplicated issues, only my issue (#65) describes the full and valid impact: Whitelisted makers that cannot be liquidated will generate bad debt on the protocol, creating a loss for all users.

You're right that the intended design is to not liquidate whitelisted makers, but that will create bad debt on the protocol, and therefore cause a loss of funds. The Sherlock docs state that design decisions are not valid issues if they don't imply any loss of funds, but this issue will imply a loss of funds and therefore should be valid.

I will remove my escalation here and I will escalate my issue (#65).

[gstoyanovbg]

Escalate

The previous escalation was removed because the Watson decided that theirs issue is not duplicate of this one. This is why i escalate it again.

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel. While considering the validity of an issue in case of any conflict the sources of truth are prioritized in the above order.

The hierarchy of truth clearly states that contest's README has more weight than code comments. This issue is not part of the known issues section of the README so it is not known issue in the context of the contest.

I believe it is a valid issue.

[sherlock-admin2]

Escalate

The previous escalation was removed because the Watson decided that theirs issue is not duplicate of this one. This is why i escalate it again.

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel. While considering the validity of an issue in case of any conflict the sources of truth are prioritized in the above order.

The hierarchy of truth clearly states that contest's README has more weight than code comments. This issue is not part of the known issues section of the README so it is not known issue in the context of the contest.

I believe it is a valid 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]

Agree with @IllIllI000 comments here, I believe it is a known issue within code comments that whitelisted maker cannot be liquidated

[WangSecurity]

Agree with LSW and the Lead Judge. The hierarchy of truth can be applied when there is conflicting information between the code and the rules, but the code and documentation can be used to define the intended design and known issues of the protocol. We understand that it's a bit confusing, but hope for your understanding. Therefore, I agree it's an intended design and a known issue.

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

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• gstoyanovbg: rejected

[gstoyanovbg]

Agree with LSW and the Lead Judge. The hierarchy of truth can be applied when there is conflicting information between the code and the rules, but the code and documentation can be used to define the intended design and known issues of the protocol. We understand that it's a bit confusing, but hope for your understanding. Therefore, I agree it's an intended design and a known issue.

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

From Watson's point view there is no way to know when this rule should be applied and when shouldn't. Everyone interpreted it differently.

// We don't allow liquidating whitelisted makers for now until we implement safety mechanism // For spot-hedged base maker, it needs to implement rebalance once it got liquidated

Nowhere is stated that this cause other issues neither in the comment or in the known issues section. Moreover this line from the rules :

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel. While considering the validity of an issue in case of any conflict the sources of truth are prioritized in the above order.

unambiguously say that the README has more weight than "protocol documentation (including code comments)". In your comment you say that

The hierarchy of truth can be applied when there is conflicting information between the code and the rules

but this is not true according to the above rule which states that the conflict may be between ALL sources of truth :

While considering the validity of an issue in case of any conflict the sources of truth are prioritized in the above order.

So i don't understand the decision - from my point of view i am penalized with worsen ratio because follow the rules and interpret them in the way they are written.

[santipu03]

@gstoyanovbg I totally agree with you, I think in this contest the judges suddenly have decided to make decisions against the current hierarchy of truth. I've got a couple of issues that have been rejected that also should be valid based on the current hierarchy, damaging my issues ratio and escalations ratio.

@WangSecurity @Evert0x If you have decided now that the hierarchy of truth must be changed, that's totally fair. However, these new changes should be applied to contests still not finished, not to a contest that has already finished like this one. I think it's greatly unfair that these changes haven't been announced anywhere and now some Watsons are suffering its consequences.

I don't mean anything here in a bad way, I honestly think Sherlock always has been characterized by its transparent and fair judging, so that's why now I'm surprised by these sudden changes in the rules. I would like to kindly ask the judges to reconsider some decisions that have been made during these last days to issues regarding "known issues" vaguely described by a few code comments. Thanks.

[IllIllI000]

@santipu03 and @gstoyanovbg while I agree that the hierarchy is unclear, it was discussed here: https://github.com/sherlock-audit/2024-03-vvv-vesting-staking-judging/issues/148#issuecomment-2045743718 and I believe the current judge is interpreting things in a similar way

[santipu03]

@santipu03 and @gstoyanovbg while I agree that the hierarchy is unclear, it was discussed here: https://github.com/sherlock-audit/2024-03-vvv-vesting-staking-judging/issues/148#issuecomment-2045743718 and I believe the current judge is interpreting things in a similar way

I think there's a fundamental difference between the referenced discussion and the scenario we have here. Even if we consider that the code comments describe this scenario as a known issue, which I don't agree btw, we still have conflicting information between the code comments and the Sherlock rules.

Simply put, I think there is a conflict because the rules are saying this issue should be a MEDIUM (because it causes a loss of funds), and the code comments are saying that this issue should be INVALID (known issue). To me, this is conflicting information and the Sherlock rules should have priority over the code comments, as the hierarchy states. CC. @WangSecurity and @Evert0x.