xiaoming90 - Position value can fall below the minimum acceptable quote value

[sherlock-admin]

xiaoming90

medium

Position value can fall below the minimum acceptable quote value

Summary

PartyB can fill a LIMIT order position till the point where the value is below the minimum acceptable quote value (minAcceptableQuoteValue). As a result, it breaks the invariant that the value of position must be above the minimum acceptable quote value, leading to various issues and potentially losses for the users.

Vulnerability Detail

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L196

File: LibQuote.sol
149:     function closeQuote(Quote storage quote, uint256 filledAmount, uint256 closedPrice) internal {
..SNIP..
189:         if (quote.closedAmount == quote.quantity) {
190:             quote.quoteStatus = QuoteStatus.CLOSED;
191:             quote.requestedClosePrice = 0;
192:             removeFromOpenPositions(quote.id);
193:             quoteLayout.partyAPositionsCount[quote.partyA] -= 1;
194:             quoteLayout.partyBPositionsCount[quote.partyB][quote.partyA] -= 1;
195:         } else if (
196:             quote.quoteStatus == QuoteStatus.CANCEL_CLOSE_PENDING || quote.quantityToClose == 0
197:         ) {
198:             quote.quoteStatus = QuoteStatus.OPENED;
199:             quote.requestedClosePrice = 0;
200:             quote.quantityToClose = 0; // for CANCEL_CLOSE_PENDING status
201:         } else {
202:             require(
203:                 quote.lockedValues.total() >=
204:                     SymbolStorage.layout().symbols[quote.symbolId].minAcceptableQuoteValue,
205:                 "LibQuote: Remaining quote value is low"
206:             );
207:         }
208:     }

If the user has already sent the close request, but partyB has not filled it yet, the user can request to cancel it by calling the CancelCloseRequest function. This will cause the quote's status to change to QuoteStatus.CANCEL_CLOSE_PENDING.

PartyB can either accept the cancel request or fill the close request ignoring the user's request. If PartyB decided to go ahead to fill the close request partially, the second branch of the if-else statement at Line 196 will be executed. However, the issue is that within this branch, PartyB is not subjected to the minAcceptableQuoteValue validation check. Thus, it is possible for PartyB to fill a LIMIT order position till the point where the value is below the minimum acceptable quote value (minAcceptableQuoteValue).

Impact

In the codebase, the minAcceptableQuoteValue is currently set to 5 USD. There are many reasons for having a minimum quote value in the first place. For instance, if the value of a position is too low, it will be uneconomical for the liquidator to liquidate the position because the liquidation fee would be too small or insufficient to cover the cost of liquidation. Note that the liquidation fee is computed as a percentage of the position value.

This has a negative impact on the overall efficiency of the liquidation mechanism within the protocol, which could delay or stop the liquidation of accounts or positions, exposing users to greater market risks, including the risk of incurring larger losses or having to exit at an unfavorable price.

Code Snippet

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L196

Tool used

Manual Review

Recommendation

If the user sends a close request and PartyB decides to go ahead to fill the close request partially, consider checking if the remaining value of the position is above the minimum acceptable quote value (minAcceptableQuoteValue) after PartyB has filled the position.

function closeQuote(Quote storage quote, uint256 filledAmount, uint256 closedPrice) internal {
	..SNIP..
    if (quote.closedAmount == quote.quantity) {
        quote.quoteStatus = QuoteStatus.CLOSED;
        quote.requestedClosePrice = 0;
        removeFromOpenPositions(quote.id);
        quoteLayout.partyAPositionsCount[quote.partyA] -= 1;
        quoteLayout.partyBPositionsCount[quote.partyB][quote.partyA] -= 1;
    } else if (
        quote.quoteStatus == QuoteStatus.CANCEL_CLOSE_PENDING || quote.quantityToClose == 0
    ) {
        quote.quoteStatus = QuoteStatus.OPENED;
        quote.requestedClosePrice = 0;
        quote.quantityToClose = 0; // for CANCEL_CLOSE_PENDING status
+        
+        require(
+            quote.lockedValues.total() >=
+                SymbolStorage.layout().symbols[quote.symbolId].minAcceptableQuoteValue,
+            "LibQuote: Remaining quote value is low"
+        );
    } else {
        require(
            quote.lockedValues.total() >=
                SymbolStorage.layout().symbols[quote.symbolId].minAcceptableQuoteValue,
            "LibQuote: Remaining quote value is low"
        );
    }
}

[MoonKnightDev]

The lockedValues amount of remaining open portion of the position is already validated in the 'requestToClose' function, so there's no need to check 'minAcceptableQuoteValue' within the 'closeQuote' function. https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/PartyA/PartyAFacetImpl.sol#L175

[hrishibhat]

Considering this a non issue based on the above sponsor comments

[shaka0x]

@hrishibhat @MoonKnightDev I do not understand the reasoning. The scenario can be:

• minAcceptableQuoteValue: 5
• It is requested to close 100% of the position, so remaining open portion is 0
• Position is closed partially and we end up with a position of value 3

[xiaoming9090]

@hrishibhat @MoonKnightDev I do not understand the reasoning. The scenario can be:

• minAcceptableQuoteValue: 5
• It is requested to close 100% of the position, so remaining open portion is 0
• Position is closed partially and we end up with a position of value 3

If someone requests to close 100% of the position, quote.closedAmount == quote.quantity. In this case, the position is closed completely and this section of the code will be executed instead.

The minAcceptableQuoteValue is not relevant for a position that is closed completely.

[shaka0x]

@hrishibhat @MoonKnightDev I do not understand the reasoning. The scenario can be:

• minAcceptableQuoteValue: 5
• It is requested to close 100% of the position, so remaining open portion is 0
• Position is closed partially and we end up with a position of value 3

If someone requests to close 100% of the position, quote.closedAmount == quote.quantity. In this case, the position is closed completely and this section of the code will be executed instead.

The minAcceptableQuoteValue is not relevant for a position that is closed completely.

I mean passing 100% in requestToClosePosition.
@MoonKnightDev says the check here will prevent the lockedValues amount of remaining open portion of the position be below the minAcceptableQuoteValue. If party A requests to close all, there are no remaining open portion, so the check passes.
Now, in closeQuote, party B closes it partially and we do end up with an open position with value below the minAcceptableQuoteValue.

[panprog]

Agree with @shaka0x
Got a working proof of concept here
partyA requestToClosePosition -> remainder can't be below minimum
partyA requestToClosePosition -> partyB fillCloseRequest (partial) -> remainder can't be below minimum
partyA requestToClosePosition -> partyA requestToCancelCloseRequest -> partyB fillCloseRequest (partial) -> remainder CAN be below minimum

[naveedinno]

Fixed code PR link:
SYMM-IO/protocol-core#20

[hrishibhat]

After further review based on the points raised by @shaka0x @panprog considering this a valid issue.
The same is fixed by the Sponsor.