berndartmueller - Liquidating a turned solvent Party A does not credit the profits to Party A

[sherlock-admin]

berndartmueller

medium

Liquidating a turned solvent Party A does not credit the profits to Party A

Summary

Party A can turn solvent again mid-way through the multi-step liquidation process. While Party B will have its losses deducted from its allocated balance, Party A will not receive any profits. Instead, its allocated balance is reset to 0.

Vulnerability Detail

If Party A turns solvent again, i.e., its available balance (availableBalance) is positive, after a liquidator has started the liquidation and calls the setSymbolsPrice to initialize the symbol prices as well as Party A's liquidation details, the liquidation will proceed as usual. Liquidating the individual open positions of Party A with the liquidatePositionsPartyA function deducts the losses from the trading counterparty B's allocated balance in line 170.

However, the profits made by Party A are not credited to Party A's allocated balance. Instead, Party A's allocated balance is reset to 0 in line 216 once all positions are liquidated.

Impact

Party A's realized profits during the liquidation are retained by the protocol instead of credited to Party A's allocated balance.

Code Snippet

contracts/facets/liquidation/LiquidationFacetImpl.sol#L65-L67

Party A, who turned solvent, will have the liquidation proceed as usual, with the liquidationType set to NORMAL.

34: function setSymbolsPrice(address partyA, PriceSig memory priceSig) internal {
...     // [...]
51:
52:     int256 availableBalance = LibAccount.partyAAvailableBalanceForLiquidation(
53:         priceSig.upnl,
54:         partyA
55:     );
56:     if (accountLayout.liquidationDetails[partyA].liquidationType == LiquidationType.NONE) {
57:         accountLayout.liquidationDetails[partyA] = LiquidationDetail({
58:             liquidationType: LiquidationType.NONE,
59:             upnl: priceSig.upnl,
60:             totalUnrealizedLoss: priceSig.totalUnrealizedLoss,
61:             deficit: 0,
62:             liquidationFee: 0
63:         });
64: @>      if (availableBalance >= 0) {
65: @>          uint256 remainingLf = accountLayout.lockedBalances[partyA].lf;
66: @>          accountLayout.liquidationDetails[partyA].liquidationType = LiquidationType.NORMAL;
67: @>          accountLayout.liquidationDetails[partyA].liquidationFee = remainingLf;
68:         } else if (uint256(-availableBalance) < accountLayout.lockedBalances[partyA].lf) {
...     // [...]
97: }

contracts/facets/liquidation/LiquidationFacetImpl.sol#L170

Liquidating Party A's positions, which are in a profit (and thus a loss for Party B), deducts the losses from Party B's allocated balance in line 170. The profit is not credited to Party A.

File: LiquidationFacetImpl.sol
126: function liquidatePositionsPartyA(
127:     address partyA,
128:     uint256[] memory quoteIds
129: ) internal returns (bool) {
130:     AccountStorage.Layout storage accountLayout = AccountStorage.layout();
131:     MAStorage.Layout storage maLayout = MAStorage.layout();
132:     QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
133:
134:     require(maLayout.liquidationStatus[partyA], "LiquidationFacet: PartyA is solvent");
135:     for (uint256 index = 0; index < quoteIds.length; index++) {
136:         Quote storage quote = quoteLayout.quotes[quoteIds[index]];
...          // [...]
162:
163:         if (
164:             accountLayout.liquidationDetails[partyA].liquidationType == LiquidationType.NORMAL
165:         ) {
166:             accountLayout.partyBAllocatedBalances[quote.partyB][partyA] += quote
167:                 .lockedValues
168:                 .cva;
169:             if (hasMadeProfit) {
170: @>              accountLayout.partyBAllocatedBalances[quote.partyB][partyA] -= amount; // @audit-info Party B's allocated balance is decreased by the amount of profit made by party A
171:             } else {
172:                 accountLayout.partyBAllocatedBalances[quote.partyB][partyA] += amount;
173:             }

contracts/facets/liquidation/LiquidationFacetImpl.sol#L216

Once all of Party A's positions are liquidated, Party A's allocated balance is reset to 0 in line 216.

126: function liquidatePositionsPartyA(
127:     address partyA,
128:     uint256[] memory quoteIds
129: ) internal returns (bool) {
...   // [...]
211:  if (quoteLayout.partyAPositionsCount[partyA] == 0) {
212:      require(
213:          quoteLayout.partyAPendingQuotes[partyA].length == 0,
214:          "LiquidationFacet: Pending quotes should be liquidated first"
215:      );
216:  @>  accountLayout.allocatedBalances[partyA] = 0;
217:      accountLayout.lockedBalances[partyA].makeZero();
218:
219:      uint256 lf = accountLayout.liquidationDetails[partyA].liquidationFee;
220:      if (lf > 0) {
221:          accountLayout.allocatedBalances[accountLayout.liquidators[partyA][0]] += lf / 2;
222:          accountLayout.allocatedBalances[accountLayout.liquidators[partyA][1]] += lf / 2;
223:      }
224:      delete accountLayout.liquidators[partyA];
225:      maLayout.liquidationStatus[partyA] = false;
226:      maLayout.liquidationTimestamp[partyA] = 0;
227:      accountLayout.liquidationDetails[partyA].liquidationType = LiquidationType.NONE;
228:      if (
229:          accountLayout.totalUnplForLiquidation[partyA] !=
230:          accountLayout.liquidationDetails[partyA].upnl
231:      ) {
232:          accountLayout.totalUnplForLiquidation[partyA] = 0;
233:          return false;
234:      }
235:      accountLayout.totalUnplForLiquidation[partyA] = 0;
236:  }
237:  return true;

Tool used

Manual Review

Recommendation

Consider adding Party A's realized profits during the liquidation to Party A's allocated balance.

[mstpr]

Escalate

This works as intended. Liquidation should happen when the price reaches the liq threshold this is how the leverage trading works. Even a small wick touching the liq threshold price should initiate an immediate liquidation for the partyA. If the price goes immediately back up to liq threshold then this account should still be liquidated because the price touched to the liquidation threshold already even for a single second.

Leverage trading between two parties are basically contracts, if partyA is liquidatable for even a millisecond it should be liquidated and partyB should be compensated because the agreement is settled already.

[sherlock-admin2]

Escalate

This works as intended. Liquidation should happen when the price reaches the liq threshold this is how the leverage trading works. Even a small wick touching the liq threshold price should initiate an immediate liquidation for the partyA. If the price goes immediately back up to liq threshold then this account should still be liquidated because the price touched to the liquidation threshold already even for a single second.

Leverage trading between two parties are basically contracts, if partyA is liquidatable for even a millisecond it should be liquidated and partyB should be compensated because the agreement is settled already.

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.

[berndartmueller]

I agree that Party A should always be liquidated, even if the liquidation threshold was touched just for a very short time.

However, the demonstrated issue here is that if Party A has multiple positions, those which are in profit are not considered in party A's balance (while Party B will have it deducted). Instead, the protocol amasses the funds while not having the ability to withdraw them.

[JeffCX]

I agree that Party A should always be liquidated, even if the liquidation threshold was touched just for a very short time.

However, the demonstrated issue here is that if Party A has multiple positions, those which are in profit are not considered in party A's balance (while Party B will have it deducted). Instead, the protocol amasses the funds while not having the ability to withdraw them.

I have to agree with the submitter's comment and recommend maintaining the medium severtiy

[mstpr]

PartyA is trading cross not isolated like partyB's. PartyA can have 5 trades going on with 5 different partyB's where 4 of them could be in profit but 1 of them is in huge loss that leads to liquidation. As you can also see in the contracts the total available balance for partyA is the cumulative locked balances + cumulative pnl which indicates that partyA is actually responsible for the overall balance of its position not individual positions as in isolated trading. Hence, this should be invalid

[Evert0x]

Party A's realized profits during the liquidation are retained by the protocol instead of credited to Party A's allocated balance.

This does seem like intended behavior and can count as a loss for other parties involved

[berndartmueller]

If Party A turned solvent mid-way through the liquidation process (between calling setSymbolsPrice and liquidatePositionsPartyA), i.e., cumulative locked balances + cumulative PnL is positive, the profits from the profitable positions are not credited to Party A, while the losses for Party B are accounted for in L170.

Those retained profits from Party A sit in the protocol's contract and remain unutilized. Besides, it seems the sponsor confirmed the issue as well. Curious to hear their thoughts.

[MoonKnightDev]

Fixed code PR link: https://github.com/SYMM-IO/symmio-core/pull/22

[mstpr]

If Party A turned solvent mid-way through the liquidation process (between calling setSymbolsPrice and liquidatePositionsPartyA), i.e., cumulative locked balances + cumulative PnL is positive, the profits from the profitable positions are not credited to Party A, while the losses for Party B are accounted for in L170.

Those retained profits from Party A sit in the protocol's contract and remain unutilized. Besides, it seems the sponsor confirmed the issue as well. Curious to hear their thoughts.

I think MM is almost always idle and retained in the protocol contract in NORMAL liquidations

say cva = 180, lf = 20, mm = 200, pnl = -201

400 - 200 - 201 = -1, partyA is liqable.

Now, say partyA is solvent midway as you said, the pnl is +1 now.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L166-L173 partyBLockedBalances += 180 partyBLockedBalances -= 1

partyBLockedBalances = 400 + 180 - 1 = 579

now, partyA's cva went to partyB partyA's lf went to liquidator partyA's mm is ?? I think this is left in the contract anyways

so we can say in a normal liquidation mm is always stucks in contract? Am I missing something?

[panprog]

so we can say in a normal liquidation mm is always stucks in contract? Am I missing something?

It's not mm that the protocol "steals", it's the partyA balance after it turns solvent. In your example if pnl changed from -201 to +1, that means partyA balance becomes: 400-200+1 = +201 - so 201 is stolen by the protocol (but it has nothing to do with mm - it's just a close number in your example). If pnl of partyA changes from -201 to -199, then partyA balance becomes 400-200-199=+1 - partyA is solvent. partyBallocatedBalance += 180 + 199 = +379 liquidator will get +20 partyA balance will be set to 0 (from 400) (-400) So the balances for all parties are: +379+20-400=-1 So 1 is retained (stolen) by the protocol.

[mstpr]

so we can say in a normal liquidation mm is always stucks in contract? Am I missing something?

It's not mm that the protocol "steals", it's the partyA balance after it turns solvent. In your example if pnl changed from -201 to +1, that means partyA balance becomes: 400-200+1 = +201 - so 201 is stolen by the protocol (but it has nothing to do with mm - it's just a close number in your example). If pnl of partyA changes from -201 to -199, then partyA balance becomes 400-200-199=+1 - partyA is solvent. partyBallocatedBalance += 180 + 199 = +379 liquidator will get +20 partyA balance will be set to 0 (from 400) (-400) So the balances for all parties are: +379+20-400=-1 So 1 is retained (stolen) by the protocol.

yes correct! Thanks for the correction!

So overall, the intended behavior was to make locked balances of partyA 0 if liquidation happens to partyA and credit loss/profit to partyB.

In this edge case you describe its true that if pnl changes like that the excess amount will be retained in protocol. However, I think the intended behaviour is to make partyA balance 0 all the time in partyA liquidations, maybe this excess can go to somewhere else I am not sure. Also, the fix implemented above is not addressing this issue.

In theory everything works supposed to and considering this issue is very unlikely to happen in normal operations (liquidators would call liquidatePartyA and setSymbols in same tx or right away to get both fees) makes this a low issue imo. However, it is clear that protocol team did not know about the stuck token part. On the other hand, if liquidations are supposed to make partyA's balance 0 regardless, then we can't really say that there is a "loss of funds" which makes the issue low/informational on Sherlock standards. I think sponsor team should say the last word here.

[hrishibhat]

Additonal Sponsor comment:

These funds belong to PartyB. Therefore, I believe it should be considered a medium.

[hrishibhat]

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

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• mstpr: rejected