xiaoming90 - Users might immediately be liquidated after position opening leading to a loss of CVA and Liquidation fee

[sherlock-admin]

xiaoming90

high

Users might immediately be liquidated after position opening leading to a loss of CVA and Liquidation fee

Summary

The insolvency check (isSolventAfterOpenPosition) within the openPosition function does not consider the locked balance adjustment, causing the user account to become insolvent immediately after the position is opened. As a result, the affected users will lose their CVA and liquidation fee locked in their accounts.

Vulnerability Detail

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L150

File: PartyBFacetImpl.sol
112:     function openPosition(
113:         uint256 quoteId,
114:         uint256 filledAmount,
115:         uint256 openedPrice,
116:         PairUpnlAndPriceSig memory upnlSig
117:     ) internal returns (uint256 currentId) {
..SNIP..
150:         LibSolvency.isSolventAfterOpenPosition(quoteId, filledAmount, upnlSig);
151: 
152:         accountLayout.partyANonces[quote.partyA] += 1;
153:         accountLayout.partyBNonces[quote.partyB][quote.partyA] += 1;
154:         quote.modifyTimestamp = block.timestamp;
155: 
156:         LibQuote.removeFromPendingQuotes(quote);
157: 
158:         if (quote.quantity == filledAmount) {
159:             accountLayout.pendingLockedBalances[quote.partyA].subQuote(quote);
160:             accountLayout.partyBPendingLockedBalances[quote.partyB][quote.partyA].subQuote(quote);
161: 
162:             if (quote.orderType == OrderType.LIMIT) {
163:                 quote.lockedValues.mul(openedPrice).div(quote.requestedOpenPrice);
164:             }
165:             accountLayout.lockedBalances[quote.partyA].addQuote(quote);
166:             accountLayout.partyBLockedBalances[quote.partyB][quote.partyA].addQuote(quote);
167:         }

The leverage of a position is computed based on the following formula.

$leverage = \frac{price \times quantity}{lockedValues.total()}$

When opening a position, there is a possibility that the leverage might change because the locked values and quantity are fixed, but it could get filled with a different market price compared to the one at the moment the user requested. Thus, the purpose of Line 163 above is to adjust the locked values to maintain a fixed leverage. After the adjustment, the locked value might be higher or lower.

The issue is that the insolvency check at Line 150 is performed before the adjustment is made.

Assume that the adjustment in Line 163 cause the locked values to increase. The insolvency check (isSolventAfterOpenPosition) at Line 150 will be performed with old or unadjusted locked values that are smaller than expected. Since smaller locked values mean that there will be more available balance, this might cause the system to miscalculate that an account is not liquidatable, but in fact, it is actually liquidatable once the adjusted increased locked value is taken into consideration.

In this case, once the position is opened, the user account is immediately underwater and can be liquidated.

The issue will occur in the "complete fill" path and "partial fill" path since both paths adjust the locked values to maintain a fixed leverage. The "complete fill" path adjusts the locked values at Line 185

Impact

Users might become liquidatable immediately after opening a position due to an incorrect insolvency check within the openPosition, which erroneously reports that the account will still be healthy after opening the position, while in reality, it is not. As a result, the affected users will lose their CVA and liquidation fee locked in their accounts.

Code Snippet

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L150

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L185

Tool used

Manual Review

Recommendation

Consider performing the insolvency check with the updated adjusted locked values.

[MoonKnightDev]

This scenario could only happen if the user requests to open a short position at a price significantly lower than the market value, creating conditions for potential liquidation. In this case, the identified bug would indeed facilitate this outcome. However, because the existence of such conditions is a prerequisite, we don't believe the severity level is high.

[ctf-sec]

Changed the severity to medium based on the comments above

[mstpr]

Escalate

This is not true. Accounts can not be immediately liquidatable in this scenario which is ensured by this function
https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L149-L150

Also, partyA can never be liquidatable after opening a position but partyB who opens the position might open the position in a level where it's almost liquidatable (-1 pnl would sufficient etc). However, partyB would not open a position because its not favor of doing so. (Who would want to open a position at a price where they are liquidatable immediately?)

Why partyA is not liquidatable immediately after partyB opens the position in such level?

Because the nonce increases in openPosition, which means that the new MuonSignature is needed. New MuonSignature will count the pnl of the partyA that its in huge profit (because partyB opened the position in a very undesired price) hence, the partyA can't be liquidatable.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L152-L153
https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/libraries/LibAccount.sol#L78-L86

Considering that, there is no point of doing something for any partyB because the harm is only to them. No body will create a position that is below/above the requested open price such that they are immediately liquidate after a small price change.

There is only 1 scenario that the partyA can be liquidatable which is only covered by this issue #77. This finding and the duplicates are missing the complex edge case.

[sherlock-admin2]

Escalate

This is not true. Accounts can not be immediately liquidatable in this scenario which is ensured by this function
https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L149-L150

Also, partyA can never be liquidatable after opening a position but partyB who opens the position might open the position in a level where it's almost liquidatable (-1 pnl would sufficient etc). However, partyB would not open a position because its not favor of doing so. (Who would want to open a position at a price where they are liquidatable immediately?)

Why partyA is not liquidatable immediately after partyB opens the position in such level?

Because the nonce increases in openPosition, which means that the new MuonSignature is needed. New MuonSignature will count the pnl of the partyA that its in huge profit (because partyB opened the position in a very undesired price) hence, the partyA can't be liquidatable.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L152-L153
https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/libraries/LibAccount.sol#L78-L86

Considering that, there is no point of doing something for any partyB because the harm is only to them. No body will create a position that is below/above the requested open price such that they are immediately liquidate after a small price change.

There is only 1 scenario that the partyA can be liquidatable which is only covered by this issue #77. This finding and the duplicates are missing the complex edge case.

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.

[MoonKnightDev]

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

[hrishibhat]

Result:
Medium
Has duplicates
Agree with the following comment from the Lead senior Watson:

The root cause/bug of this report and its duplicates is that the solvency check is performed against the old locked values instead of the adjusted/actual locked values. So when the solvency check is performed against the old locked values, it might underestimate and assumes everything is well. However, the position is opened with the adjusted/actual locked values, not the old locked values. So the position might end up opening at a higher price (it was underestimated earlier), resulting the account to be liquidatable,

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• mstpr: rejected