xiaoming90 - Force Close can be DOSed by exploiting `settleUpnl` function

[sherlock-admin4]

xiaoming90

Medium

Force Close can be DOSed by exploiting settleUpnl function

Summary

PartyB can abuse the newly implemented settleUpnl function to increment the nonce to block PartyA's force close action.

Root Cause

1. A SettlementSig signature with quotesSettlementsData and/or partyBs array being empty can be passed into settleUpnl function
2. No cooldown if PartyB is settling its own positions
3. No minimum PnL to be settled (Even 1 wei of PnL can be settled)

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

This report is similar to the report from the previous contest, but with a different attack vector.

Assume that PartyA requests to close a quote via requestToClosePosition function. If PartyB does not respond within the cooldown period, PartyA can call the PartyAFacetImpl.forceClosePosition to close the quote forcefully.

Within the forceClosePosition function:

• In Line 86 below, the verifyHighLowPrice function will verify that the signature is valid. The function will use the nonce of PartyA and PartyB to generate the hash.
• In Line 88 below, the verifySettlement function verifies that the signature is valid. Similarly, it also relies on the nonce of PartyA and PartyB to generate the hash.

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/ForceActions/ForceActionsFacetImpl.sol#L86

File: ForceActionsFacetImpl.sol
51:     function forceClosePosition(
52:         uint256 quoteId,
53:         HighLowPriceSig memory sig,
54:         SettlementSig memory settlementSig,
55:         uint256[] memory updatedPrices
56:     ) internal returns (uint256 closePrice, bool isPartyBLiquidated, int256 upnlPartyB, uint256 partyBAllocatedBalance) {
..SNIP..
86:         LibMuonForceActions.verifyHighLowPrice(sig, quote.partyB, quote.partyA, quote.symbolId);
87:         if (updatedPrices.length > 0) {
88:             LibMuonSettlement.verifySettlement(settlementSig, quote.partyA);
89:         }

Attack Path 1 - Incrementing PartyA's nonce

The following describes the attack path:

1. At this point, PartyA's nonce is 200. PartyA fetches the signatures from Muon and bundles them with the forceClosePosition transaction, and submits to the mempool to close the quote forcefully as PartyB did not respond to PartyA's close request for an extended period.
2. PartyB front-run PartyA's transaction and exploit the new settleUpnl function to increment its nonce to 201.
3. When PartyA's transaction gets executed, since the signature's PartyA nonce is 200, while the PartyA's nonce on-chain is 201, the transaction will revert.
4. Party B could continuously grieve Party A, preventing them from being forced to close their positions.

There are several ways that PartyB can increment the PartyA's nonce via the settleUpnl function with no cost or minimum cost:

1. Passing a SettlementSig signature with quotesSettlementsData and/or partyBs array being empty. In this case, the code at Line 34 will increment PartyA's nonce, but the rest of the code that involves the for-loop will be bypassed since the array (settleSig.quotesSettlementsData.length = 0) is empty

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

File: LibSettlement.sol
15:     function settleUpnl(
16:         SettlementSig memory settleSig,
..SNIP..
34:         accountLayout.partyANonces[partyA] += 1;
..SNIP..
40:         for (uint8 i = 0; i < settleSig.quotesSettlementsData.length; i++) {

2. The tricks mentioned in "Attack Path 2" section below for incrementing PartyB's nonce can also be applied here to increment PartyA's nonce. Refer to the next section for more details.

Attack Path 2 - Incrementing PartyB's nonce

The following describes the attack path:

1. At this point, PartyB's nonce is 800. PartyA fetches the signatures from Muon and bundles them with the forceClosePosition transaction, and submits to the mempool to close the quote forcefully as PartyB did not respond to PartyA's close request for an extended period.
2. PartyB front-run PartyA's transaction and exploit the new settleUpnl function to increment its nonce to 801.
3. When PartyA's transaction gets executed, since the signature's PartyB nonce is 800, while the PartyB's nonce on-chain is 801, the transaction will revert.
4. Party B could continuously grieve Party A, preventing them from being forced to close their positions.

There are several ways that PartyB can increment its own nonce via the settleUpnl function with no cost or minimum cost:

1. PartyA is permissionless. PartyB can create a PartyA account and open some dummy positions with its account. When PartyB needs to increment its nonce, simply settle a minimum possible PnL between these two accounts. There is no cooldown since PartyB is settling its own positions.
2. If PartyB already has existing positions, PartyB can settle a minimum possible PnL on these positions. There is no cooldown since PartyB is settling its own positions.

Impact

This impact is the same as the impact of a report in the previous Symm IO contest. Thus, the risk rating should be aligned to Medium.

PartyB can take advantage of it against PartyA, making themselves always profitable. For instance:

1. If the current price goes for PartyB, then the quote is closed, and PartyB makes a profit.
2. If the current price goes against PartyB, PartyB can front-run forceClosePosition and call settleUpnl to increase PartyA's nonces. In this way, PartyA's forceClosePosition will inevitably revert because the nonces are incorrect.

In addition, when PartyA cannot close its position promptly, it exposes PartyA to unnecessary market risks and potential losses and gives PartyB an unfair advantage by giving PartyB an opportunity to turn the table (e.g., loss -> profit).

PoC

No response

Mitigation

Consider implementing the following measures to mitigate the root causes:

1. Ensure that SettlementSig signature with quotesSettlementsData and/or partyBs array being empty are rejected within settleUpnl function
2. Implement some cooldown even if PartyB is settling its own positions
3. Implement a minimum PnL to be settled so that no one can attempt to abuse the settle upnl feature by only settling 1 wei or small amount of PnL