shaka - Wrong calculation of solvency after request to close and after close position

[sherlock-admin]

shaka

medium

Wrong calculation of solvency after request to close and after close position

Summary

isSolventAfterClosePosition and isSolventAfterRequestToClosePosition do not account for the extra profit that the user would get from closing the position.

Vulnerability Detail

When a party A creates a request for closing a position, the isSolventAfterRequestToClosePosition function is called to check if the user is solvent after the request. In the same way, when someone tries to close a position, the isSolventAfterClosePosition function is called to check if both party A and party B are solvent after closing the position.

Both functions calculate the available balance for party A and party B, and revert if it is lower than zero. After that, the function accounts for the the extra loss that the user would get as a result of the difference between closePrice and upnlSig.price, and checks if the user is solvent after that.

The problem is that the function does not account for the opposite case, that is the case where the user would get an extra profit as a result of the difference between closePrice and upnlSig.price. This means that the user would not be able to close the position, even if at the end of the transaction they would be solvent.

Proof of Concept

There is an open position with:

• Position type: LONG
• Quantity: 1
• Locked: 50
• Opened price: 100
• Current price: 110
• Quote position uPnL Party A: 10

Party B calls fillCloseRequest with:

• Closed price: 120

In isSolventAfterClosePosition the following is calculated:

partyAAvailableBalance = freeBalance + upnl + unlockedAmount = -5

And it reverts on:

require(
    partyBAvailableBalance >= 0 && partyAAvailableBalance >= 0,
    "LibSolvency: Available balance is lower than zero"
);

However, the extra profit for closedPrice - upnlSig.price = 120 - 110 = 10 is not accounted for in the partyAAvailableBalance calculation, that should be partyAAvailableBalance = - 5 + 10 = 5. Party A would be solvent after closing the position, but the transaction reverts.

Impact

In a situation where the difference between the closed price and the current price will make the user solvent, users will not be able to close their positions, even if at the end of the transaction they would be solvent.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Add the extra profit to the partyAAvailableBalance calculation.

[MoonKnightDev]

The line that is checking whether the available balance is less than zero or not and reverts, is assessing the user's status if the position were to close right now. If it doesn't meet these conditions and doesn't pass, it implies that the user is already insolvent.

[shaka0x]

Escalate.

Even if the user is not solvent at the beginning of the close process, I cannot find any reason for not allowing a user to close a position if the outcome is that they are not insolvent anymore and both parties receive what they are due, in the same way as a user may increase their collateral to recover solvency.

[sherlock-admin2]

Escalate.

Even if the user is not solvent at the beginning of the close process, I cannot find any reason for not allowing a user to close a position if the outcome is that they are not insolvent anymore and both parties receive what they are due, in the same way as a user may increase their collateral to recover solvency.

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.

[panprog]

I agree with @shaka0x that this is a valid issue, even though very unlikely, but still possible, and it's more fair to let partyB close position with a price more favorable to partyA than requested, if it leads to solvent accounts after closure (even if partyA is insolvent at the current price).

On the other hand, I also understand the other point of view: if the user is already insolvent at the current price, he can be denied all position actions which will also be fair (as he's insolvent - he should be liquidated and no other actions allowed).

Ultimately I think this is a design choice, so it's up to developers to decide what is the expected behavior.

[MoonKnightDev]

The system is designed to verify the user's solvency after a request to close, but I agree that this step may be unnecessary. It might be sufficient to check solvency solely within the fillCloseRequest() function.

[panprog]

The system is designed to verify the user's solvency after a request to close, but I agree that this step may be unnecessary. It might be sufficient to check solvency solely within the fillCloseRequest() function.

The report is not about user being insolvent at the time of request to close, it's about user being insolvent at market price, but solvent at the requested price (during request to close) or at the closePrice (during fill close request).

During request (long position):

• Current price is $1000. User is insolvent at this price, but solvent at price $1010. He requests to close at $1010, but transaction reverts (although it should allow user request to be closed at price $1010)

During fillCloseRequest:

• User requested to close at market price $1000, he was solvent at that time
• By the time partyB calls fillCloseRequest, market price is still $1000, but user is insolvent at price $1000 (due to the other positions he has), but is solvent at price $1010.
• PartyB calls fillCloseRequest with a closePrice = $1010 (when market price = $1000). User is solvent at price $1010 and position should close successfully, but it reverts because user is insolvent at current market price ($1000).

[MoonKnightDev]

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

[hrishibhat]

Result:
Medium
Unique
Considering this a valid medium based on the above comments

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• shaka0x: accepted