sherlock-audit / 2024-09-symmio-v0-8-4-update-contest-judging

0 stars 0 forks source link

xiaoming90 - PartyA can exploit the force close opportunity to settle other positions' uPNL that they have with other PartyBs/hedgers #45

Open sherlock-admin3 opened 1 month ago

sherlock-admin3 commented 1 month ago

xiaoming90

High

PartyA can exploit the force close opportunity to settle other positions' uPNL that they have with other PartyBs/hedgers

Summary

PartyA can exploit the force close opportunity to settle the UPNL of other positions that they have with other PartyBs/hedgers. As a result, by settling the UPNL early without other hedgers' (e.g., PartyB_2, PartyB_3, PartyB_4) consent, PartyA can lock in the profit for profitable positions immediately at a price favorable to PartyA. This could be aggravated in a highly volatile market with sudden market and price fluctuations. Since this is a zero-sum game, PartyA's gain is the counterparties' loss, leading to a loss of assets for the PartyB/hedgers.

Root Cause

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

The Symm IO v0.8.3 documentation describes the intention of allowing PartyA (users) to force-close the hedger's positions.

a user might want to force-close a position, but the hedger doesn't have sufficient allocated balance. For these scenarios, we've added a method called settleAndForceClosePosition.

Assume the following scenario:

If PartyB_1 does not respond to PartyA's close request within a specific timeframe, PartyA can perform a force close against PartyB_1 via the forceClosePosition function. Within the forceClosePosition function, PartyA's can utilize the newly implemented settleUpnl function to settle the PnL of PartyB_1 to ensure that PartyB_1 has sufficient allocated balance to ensure that the force close proceeds without a revert.

However, a malicious PartyA can exploit this opportunity to also settle the UPNL of other positions that they have with other PartyBs/hedgers (e.g., PartyB_2, PartyB_3, PartyB_4) besides PartyB_1.

By settling the UPNL early without other hedgers' (e.g., PartyB_2, PartyB_3, PartyB_4) consents, PartyA can lock in the profit for profitable positions immediately at the current price. Moreover, PartyA could lock in the profit without going through the actual process of submitting a requestToClosePosition and waiting for PartyB to fill its close request, bypassing the standard position closing process.

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/libraries/LibSettlement.sol#L42

File: LibSettlement.sol
15:     function settleUpnl(
16:         SettlementSig memory settleSig,
17:         uint256[] memory updatedPrices,
18:         address partyA,
19:         bool isForceClose
20:     ) internal returns (uint256[] memory newPartyBsAllocatedBalances) {
..SNIP..
40:         for (uint8 i = 0; i < settleSig.quotesSettlementsData.length; i++) {
41:             QuoteSettlementData memory data = settleSig.quotesSettlementsData[i];
42:             Quote storage quote = quoteLayout.quotes[data.quoteId];
43:             require(quote.partyA == partyA, "LibSettlement: PartyA is invalid");
..SNIP..
68:             if (quote.positionType == PositionType.LONG) {
69:                 settleAmounts[data.partyBUpnlIndex] +=
70:                     ((int256(updatedPrices[i]) - int256(quote.openedPrice)) * int256(LibQuote.quoteOpenAmount(quote))) /
71:                     1e18;
72:             } else {
73:                 settleAmounts[data.partyBUpnlIndex] +=
74:                     ((int256(quote.openedPrice) - int256(updatedPrices[i])) * int256(LibQuote.quoteOpenAmount(quote))) /
75:                     1e18;
76:             }
77:             quote.openedPrice = updatedPrices[i];
78:         }

Impact

By settling the UPNL early without other hedgers' (e.g., PartyB_2, PartyB_3, PartyB_4) consent, PartyA can lock in the profit for profitable positions immediately at a price favorable to PartyA. This could be aggravated in a highly volatile market with sudden market and price fluctuations.

Since this is a zero-sum game, PartyA's gain is the counterparties' loss, leading to a loss of assets for the PartyB/hedgers.

Moreover, PartyA could lock in the profit without going through the actual process of submitting a requestToClosePosition and waiting for PartyB to fill its close request, bypassing the standard position closing process.

PoC

No response

Mitigation

Ensure that the forceClosePosition function only allows PartyA to settle the positions of PartyB that failed to respond to close requests within the specific timeframe. In the report's example, PartyA should only be allowed to settle PartyB_1's positions and should not be allowed to settle positions of other PartyBs/hedgers (e.g., PartyB_2, PartyB_3, PartyB_4).

There is no need for PartyA to close the positions of other PartyBs that are not related to the current position/quote to be force closed. In the report's example, if PartyB_1 has insufficient allocated balance, settleUpnl can be executed to increase its allocated balance by settling PartyB_1's PnL. Settling PnL of other hedgers' (e.g., PartyB_2, PartyB_3, PartyB_4) will do nothing to increase PartyB_1's allocated balance, except opening a new attack vector for exploiters to abuse.

If PartyA themselves do insufficient allocated balance, they should go through the standard closing process (requestToClosePosition) to close their existing winning positions with other PartyBs (e.g., PartyB_2, PartyB_3, PartyB_4) to increase the allocated balance OR deposit more funds to their account.

MoonKnightDev commented 1 month ago

There is no issue with settling positions that are in a loss. Settling a position means that the unrealized P&L (uPnL) is converted into realized P&L (PnL) for the parties involved, without affecting the trading rules.