xiaoming90 - PartyB can settle PartyA’s losing positions even when it is unnecessary and detrimental to Party A’s allocated balance

[sherlock-admin4]

xiaoming90

High

PartyB can settle PartyA’s losing positions even when it is unnecessary and detrimental to Party A’s allocated balance

Summary

PartyB can settle PartyA’s losing positions, even when it is unnecessary and only further decreases PartyA's allocated balance. This unnecessary and premature settling causes PartyA to realize losses at unfavorable market conditions and prices, without giving them the opportunity to react or potentially recover from the temporary downturn. As a result, Party A is unfairly exposed to significant financial harm, having their positions closed at the worst possible moments, leading to a loss of assets for them.

Root Cause

• Losing positions of PartyA can be closed by PartyB even though it is unnecessary and only further decrease PartyA's allocated balance

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Per Symm IO v0.8.3 documentation, the purpose of the newly implemented settleUpnl function is as follows:

allows hedgers to settle a portion of the user's unrealized PNL, effectively converting it into realized PNL. This enables the hedger to fill the user's close requests, even when the user lacks sufficient allocated balance.

This new feature is intended to solve the problem where a hedger cannot fill PartyA's close request because PartyA does not have enough allocated balance, as described in the documentation's example. As shown in the documentation's example, only PartyA's winning positions (those with positive PnL) need to be settled. Once these winning positions are settled, PartyA's allocated balance will increase, and there will be sufficient allocated balance in PartyA's account to execute PartyA's close request for its losing positions.

With that in mind, it does not make sense to allow hedgers to settle the losing positions of PartyA under any circumstance because:

• It will only further decrease PartyA's allocated balance
• Does not in any manner help the hedger to achieve the goal of fulfilling PartyA's close request
• Open up an additional attack vector for users to abuse/exploit

However, it was observed that the settleUpnl allows hedgers to settle the losing positions of PartyA.

Assume that PartyA's LONG position with symbol=ETH, quote.openedPrice = 3000 USDC, quote.quantity = 1, and data.currentPrice = 2000 USC. This position is losing position for PartyA as the ETH's price dropped after the position was opened. However, none of the price validation checks at Lines 57-67 below prevents PartyB from settling a losing position of PartyA.

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

File: LibSettlement.sol
15: 	function settleUpnl(
..SNIP..
57: 			if (quote.openedPrice > data.currentPrice) {
58: 				require(
59: 					updatedPrices[i] < quote.openedPrice && updatedPrices[i] >= data.currentPrice,
60: 					"LibSettlement: Updated price is out of range"
61: 				);
62: 			} else {
63: 				require(
64: 					updatedPrices[i] > quote.openedPrice && updatedPrices[i] <= data.currentPrice,
65: 					"LibSettlement: Updated price is out of range"
66: 				);
67: 			}
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];

Impact

PartyB can settle PartyA’s losing positions, even when it is unnecessary and only further decreases PartyA's allocated balance. This unnecessary and premature settling causes PartyA to realize losses at unfavorable market conditions and prices, without giving them the opportunity to react or potentially recover from the temporary downturn. As a result, Party A is unfairly exposed to significant financial harm, having their positions closed at the worst possible moments, leading to a loss of assets for them.

PoC

No response

Mitigation

Update the settleUpnl function to ensure that only positions/quotes that increase the target account's allocated balance can be settled. In this report's example, PartyB should only be allowed to settle PartyA's winning positions, as only this settlement will increase PartyA's allocated balance.

[MoonKnightDev]

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. On the other hand, Party Bs may also need to realize their profits