Force close can be blocked or delayed by malicious PartyB

Summary

Force close can be blocked or delayed by malicious PartyB. In the worst case, PartyB could delay until the current close request has expired. Then, PartyA would have to initial a new close request and wait for the force cooldown to pass again. In this case, even if the market price has reached the user-request price during the previous close request period, the users have to close their position with the prices from the new close request period onwards ( quote.statusModifyTimestamp + maLayout.forceCloseFirstCooldown) onwards due to the check here, which might be a more unfavorable closing price.

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).

Vulnerability Detail

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 forcefully close the quote.

In Line 246, the LibMuon.verifyHighLowPrice function below will verify that the signature is valid. Within the LibMuon.verifyHighLowPrice function, it will use the nonce of PartyA and PartyB to generate the hash.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/PartyA/PartyAFacetImpl.sol#L246

File: PartyAFacetImpl.sol
214:    function forceClosePosition(
215:        uint256 quoteId,
216:        HighLowPriceSig memory sig
217:    ) internal returns (uint256 closePrice, bool isPartyBLiquidated, int256 upnlPartyB, uint256 partyBAllocatedBalance) {
..SNIP..
246:        LibMuon.verifyHighLowPrice(sig, quote.partyB, quote.partyA, quote.symbolId);

In Line 17 of the chargeFundingRate function below, the code will validate that the quoteIds.length is more than zero. This is to guard against malicious PartyB from passing in an empty quoteIds array to increment PartyA's nonce, causing some operations to fail, as described here. With the quoteIds.length > 0 condition in the validation check, malicious PartyB can no longer use empty quoteIds array to call the chargeFundingRate function freely to increase PartyA's nonce.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/FundingRate/FundingRateFacetImpl.sol#L17

File: FundingRateFacetImpl.sol
15:     function chargeFundingRate(address partyA, uint256[] memory quoteIds, int256[] memory rates, PairUpnlSig memory upnlSig) internal {
16:         LibMuon.verifyPairUpnl(upnlSig, msg.sender, partyA);
17:         require(quoteIds.length == rates.length && quoteIds.length > 0, "PartyBFacet: Length not match");
18:         int256 partyBAvailableBalance = LibAccount.partyBAvailableBalanceForLiquidation(upnlSig.upnlPartyB, msg.sender, partyA);
19:         int256 partyAAvailableBalance = LibAccount.partyAAvailableBalanceForLiquidation(
20:             upnlSig.upnlPartyA,
21:             AccountStorage.layout().allocatedBalances[partyA],
22:             partyA
23:         );
24:         uint256 epochDuration;
25:         uint256 windowTime;
26:         for (uint256 i = 0; i < quoteIds.length; i++) {
                        // Charge Funding Rate
72:         }
73:         require(partyAAvailableBalance >= 0, "PartyBFacet: PartyA will be insolvent");
74:         require(partyBAvailableBalance >= 0, "PartyBFacet: PartyB will be insolvent");
75:         AccountStorage.layout().partyBNonces[msg.sender][partyA] += 1;
76:         AccountStorage.layout().partyANonces[partyA] += 1;
77:     }

If PartyB can increment PartyA's nonce whenever they wish, they take advantage of it against PartyA, making themselves always profitable. For instance:

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

After further review, it was found that the risk was not fully mitigated by the quoteIds.length > 0 condition. Assume a malicious PartyB has many positions with the PartyA (e.g., PartyB has 50 positions with PartyA). PartyB could rotate among all their positions with PartyA, and execute the chargeFundingRate with only one (1) quote each time. If PartyB has 50 positions with PartyA, they can block PartyA's forceClosePosition around 50 times within the funding rate window.

PartyA can potentially call the forceClosePosition function multiple times in a single block, but the nonce in the liquidation signature will be the same in all the forceClosePosition transactions, so the outcome is the same when malicious PartyB invalidates the nonce.

Since PartyA can only effectively execute one forceClosePosition TX within a single block, PartyB could delay PartyA's force close by (12 seconds * number of quotes), where 12 seconds is the average Ethereum slot's time, at the minimum. The more positions PartyB has, the longer PartyB could delay the force close. Technically, there is no restriction in the protocol that limits the number of positions a PartyB can open with a PartyA.

Impact

Medium impact as per the original report.

In the worst case, PartyB could delay until the current close request has expired. Then, PartyA would have to initial a new close request and wait for the force cooldown to pass again. In this case, even if the market price has reached the user-request price during the previous close request period, the users have to close their position with the prices from the new close request period onwards ( quote.statusModifyTimestamp + maLayout.forceCloseFirstCooldown) onwards due to the check here, which might be a more unfavorable closing price.

Code Snippet

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/PartyA/PartyAFacetImpl.sol#L246

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/FundingRate/FundingRateFacetImpl.sol#L17

Tool used

Manual Review

Recommendation

Assume that the window time is 30 minutes. In the current setup, PartyB can trigger a charge funding rate for each quote every 30 minutes. As a result, PartyB can distribute the quotes across the 30-minute timeframe, allowing them to frequently trigger charge funding against a single PartyA.

For each PartyA/PartyB pair, consider only allowing a PartyB to charge a PartyA once every window (e.g., 30 minutes). PartyB should prepare a list of all quotes for which it wants to charge the funding rate and submit them at the first second of the window.

sherlock-audit / 2024-06-symmetrical-update-2-judging

xiaoming90 - Force close can be blocked or delayed by malicious PartyB #13