sherlock-admin commented 1 year ago

xiaoming90

high

Malicious PartyB can block unfavorable close position requests causing a loss of profits for PartyB

Summary

Malicious PartyB can block close position requests that are unfavorable toward them by intentionally choose not to fulfill the close request and continuously prolonging the force close position cooldown period, causing a loss of profits for PartyA.

Vulnerability Detail

If PartyA invokes the requestToClosePosition function for an open quote, the quote's status will transition from QuoteStatus.OPEN to QuoteStatus.CLOSE_PENDING. In case PartyB fails to fulfill the close request (fillCloseRequest) during the cooldown period (maLayout.forceCloseCooldown), PartyA has the option to forcibly close the quote by utilizing the forceClosePosition function.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyA/PartyAFacetImpl.sol#L261

File: PartyAFacetImpl.sol
253:     function forceClosePosition(uint256 quoteId, PairUpnlAndPriceSig memory upnlSig) internal {
254:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
255:         MAStorage.Layout storage maLayout = MAStorage.layout();
256:         Quote storage quote = QuoteStorage.layout().quotes[quoteId];
257: 
258:         uint256 filledAmount = quote.quantityToClose;
259:         require(quote.quoteStatus == QuoteStatus.CLOSE_PENDING, "PartyAFacet: Invalid state");
260:         require(
261:             block.timestamp > quote.modifyTimestamp + maLayout.forceCloseCooldown,
262:             "PartyAFacet: Cooldown not reached"
263:         );
..SNIP..

Nevertheless, malicious PartyB can intentionally choose not to fulfill the close request and can continuously prolong the quote.modifyTimestamp, thereby preventing PartyA from ever being able to activate the forceClosePosition function.

Malicious PartyB could extend the quote.modifyTimestamp via the following steps:

1) Line 282 of the fillCloseRequest show that it is possible to partially fill a close request. As such, calls the fillCloseRequest function with the minimum possible filledAmount for the purpose of triggering the LibQuote.closeQuote function at Line 292.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L256

File: PartyBFacetImpl.sol
256:     function fillCloseRequest(
257:         uint256 quoteId,
258:         uint256 filledAmount,
259:         uint256 closedPrice,
260:         PairUpnlAndPriceSig memory upnlSig
261:     ) internal {
..SNIP..
281:         if (quote.orderType == OrderType.LIMIT) {
282:             require(quote.quantityToClose >= filledAmount, "PartyBFacet: Invalid filledAmount");
283:         } else {
284:             require(quote.quantityToClose == filledAmount, "PartyBFacet: Invalid filledAmount");
285:         }
..SNIP..
292:         LibQuote.closeQuote(quote, filledAmount, closedPrice);
293:     }

Once the LibQuote.closeQuote function is triggered, Line 153 will update the quote.modifyTimestamp to the current timestamp, which effectively extends the cooldown period that PartyA has to wait before allowing to forcefully close the position.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibQuote.sol#L149

File: LibQuote.sol
149:     function closeQuote(Quote storage quote, uint256 filledAmount, uint256 closedPrice) internal {
150:         QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
151:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
152: 
153:         quote.modifyTimestamp = block.timestamp;
..SNIP..

Impact

PartyB has the ability to deny users from forcefully closing their positions by exploiting the issue. Malicious PartyB could abuse this by blocking PartyA from closing their positions against them when the price is unfavorable toward them. For instance, when PartyA is winning the game and decided to close some of its positions against PartyB, PartyB could block the close position request to deny PartyA of their profits and prevent themselves from losing the game.

Code Snippet

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyA/PartyAFacetImpl.sol#L261

Tool used

Manual Review

Recommendation

The quote.modifyTimestamp is updated to the current timestamp in many functions, including the closeQuote function, as shown in the above example. A quick search within the codebase shows that there are around 17 functions that update the quote.modifyTimestamp to the current timestamp when triggered. Each of these functions serves as a potential attack vector for malicious PartyB to extend the quote.modifyTimestamp and deny users from forcefully closing their positions

It is recommended not to use the quote.modifyTimestamp for the purpose of determining if the force close position cooldown has reached, as this variable has been used in many other places. Instead, consider creating a new variable, such as quote.requestClosePositionTimestamp solely for the purpose of computing the force cancel quote cooldown.

The following fixes will prevent malicious PartyB from extending the cooldown period since the quote.requestClosePositionTimestamp variable is only used solely for the purpose of determining if the force close position cooldown has reached.

function requestToClosePosition(
    uint256 quoteId,
    uint256 closePrice,
    uint256 quantityToClose,
    OrderType orderType,
    uint256 deadline,
    SingleUpnlAndPriceSig memory upnlSig
) internal {
..SNIP..
    accountLayout.partyANonces[quote.partyA] += 1;
    quote.modifyTimestamp = block.timestamp;
+   quote.requestCancelQuoteTimestamp = block.timestamp;

function forceClosePosition(uint256 quoteId, PairUpnlAndPriceSig memory upnlSig) internal {
    AccountStorage.Layout storage accountLayout = AccountStorage.layout();
    MAStorage.Layout storage maLayout = MAStorage.layout();
    Quote storage quote = QuoteStorage.layout().quotes[quoteId];

    uint256 filledAmount = quote.quantityToClose;
    require(quote.quoteStatus == QuoteStatus.CLOSE_PENDING, "PartyAFacet: Invalid state");
    require(
-       block.timestamp > quote.modifyTimestamp + maLayout.forceCloseCooldown,
+       block.timestamp > quote.requestCancelQuoteTimestamp + maLayout.forceCloseCooldown,
        "PartyAFacet: Cooldown not reached"
    );

In addition, review the forceClosePosition function and applied the same fix to it since it is vulnerable to the same issue, but with a different impact.

hrishibhat commented 1 year ago

@MoonKnightDev

hrishibhat commented 1 year ago

Considering this a valid medium based on trust assumptions of partyB

CodingNameKiki commented 1 year ago

Escalate

The severity should be high:

Malicious party B can permanently prevent force closing a position, causing loss of profits for Party A.

Party Bs aren't intended to be trusted authorities and it can be seen both on the below screenshot and the screenshot at the end.

Malicious Party B is able to permanently prevent force closing a position by partially closing dust amounts.

Reference: #69

Short explanation

MARKET - requires all of the quantity to be closed LIMIT - can be partially closed

Since MARKET order is not allowed from force closing the position, the only way would be to set the order type as LIMIT which is exploitable by Malicious Party B.

PartyA requests MARKET order close
PartyB doesn't respond (malicious)
Market close expires (can't be force closed as a position)
PartyA requests a LIMIT order close
PartyB (malicious) partially closes dust amounts in order to reset the quote.modifyTimestamp to the current block.timestamp

In the end Party A doesn't have a way to close the position.

Conclusion

Party A can only force close position with LIMIT order, as MARKET is not allowed.
Party B can close dust amounts and permanently prevent force closing a position.

As mentioned by the senior watson duo to this issue - Malicious PartyB is able to permanently block unfair closing request towards them, by closing dust amounts in order to update the modify.timestamp, Party B is able to continuously prolonging the force close position cooldown period, causing a loss of profits for PartyA. In the end not even the function forceClosePosition won't be able to save Party A.

As force closing a position on MARKET order isn't allowed, the malicious Party B can partially close dust amounts to extend the forceCooldown, making the LIMIT order not force closable as well. In the end Party A won't be able to close the position.

In a case of malicious Party B, even the backup plan described by the sponsor won't work here.

sherlock-admin2 commented 1 year ago

Escalate

The severity should be high:

Malicious party B can permanently prevent force closing a position, causing loss of profits for Party A.

Party Bs aren't intended to be trusted authorities and it can be seen both on the below screenshot and the screenshot at the end.

Malicious Party B is able to permanently prevent force closing a position by partially closing dust amounts.

Reference: #69

Short explanation

MARKET - requires all of the quantity to be closed LIMIT - can be partially closed

Since MARKET order is not allowed from force closing the position, the only way would be to set the order type as LIMIT which is exploitable by Malicious Party B.

PartyA requests MARKET order close

PartyB doesn't respond (malicious)

Market close expires (can't be force closed as a position)

PartyA requests a LIMIT order close

PartyB (malicious) partially closes dust amounts in order to reset the quote.modifyTimestamp to the current block.timestamp

In the end Party A doesn't have a way to close the position.

Conclusion

Party A can only force close position with LIMIT order, as MARKET is not allowed.

Party B can close dust amounts and permanently prevent force closing a position.

As mentioned by the senior watson duo to this issue - Malicious PartyB is able to permanently block unfair closing request towards them, by closing dust amounts in order to update the modify.timestamp, Party B is able to continuously prolonging the force close position cooldown period, causing a loss of profits for PartyA. In the end not even the function forceClosePosition won't be able to save Party A.

As force closing a position on MARKET order isn't allowed, the malicious Party B can partially close dust amounts to extend the forceCooldown, making the LIMIT order not force closable as well. In the end Party A won't be able to close the position.

In a case of malicious Party B, even the backup plan described by the sponsor won't work here.

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.

CodingNameKiki commented 1 year ago

Additional information from the sponsor.

Leaving the decision to the Sherlock team.

hrishibhat commented 1 year ago

Result: Medium Has duplicates Maintaining the severity of the issue, as partyB currently is a registered entity along with the additional context of the mentioned above where partyB is malicious. https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/control/ControlFacet.sol#L59 Not penalizing the escalation because there might have been some ambiguity around the trust assumptions.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

CodingNameKiki: accepted

MoonKnightDev commented 1 year ago

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

sherlock-audit / 2023-06-symmetrical-judging

xiaoming90 - Malicious PartyB can block unfavorable close position requests causing a loss of profits for PartyB #224

Malicious PartyB can block unfavorable close position requests causing a loss of profits for PartyB

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Party Bs aren't intended to be trusted authorities and it can be seen both on the below screenshot and the screenshot at the end.

Malicious Party B is able to permanently prevent force closing a position by partially closing dust amounts.

Short explanation

Conclusion

As force closing a position on MARKET order isn't allowed, the malicious Party B can partially close dust amounts to extend the forceCooldown, making the LIMIT order not force closable as well. In the end Party A won't be able to close the position.

Party Bs aren't intended to be trusted authorities and it can be seen both on the below screenshot and the screenshot at the end.

Malicious Party B is able to permanently prevent force closing a position by partially closing dust amounts.

Short explanation

Conclusion

As force closing a position on MARKET order isn't allowed, the malicious Party B can partially close dust amounts to extend the forceCooldown, making the LIMIT order not force closable as well. In the end Party A won't be able to close the position.