nikhil840096 - Deadline Bypass in Force Close Mechanism Leading to Exploitation

[sherlock-admin3]

nikhil840096

Medium

Deadline Bypass in Force Close Mechanism Leading to Exploitation

Summary

partyB can make losses to partyA by not completing the partyA request to close quote. Also making partyA, not to able to call the forceClose function

Root Cause

Protocol Design Explained

Look at the function ForceActionsFacetImpl::forceClosePosition(). It allows Party A to force close their position by referencing data before the block.timestamp with some checks. Here are those checks:

require(sig.endTime + maLayout.forceCloseSecondCooldown <= quote.deadline, "PartyBFacet: Close request is expired");
require(sig.startTime >= quote.statusModifyTimestamp + maLayout.forceCloseFirstCooldown, "PartyAFacet: Cooldown not reached");
require(sig.endTime <= block.timestamp - maLayout.forceCloseSecondCooldown, "PartyAFacet: Cooldown not reached");

Let’s take a simple example:

1. Party A wants to close their position and calls PartyAFacet::requestToClosePosition(), which will make some changes.
   Let's assume the following data:
   • maLayout.forceCloseFirstCooldown = 60
   • maLayout.forceCloseSecondCooldown = 60
   • quote.statusModifyTimestamp = block.timestamp = 314
   • quote.deadline = deadline = 500

quote.statusModifyTimestamp = block.timestamp;
quote.quoteStatus = QuoteStatus.CLOSE_PENDING;
quote.requestedClosePrice = closePrice;
quote.quantityToClose = quantityToClose;
quote.orderType = orderType;
quote.deadline = deadline;

2. Now, at block.timestamp = 502, the user selects a timeframe from 380 (sig.startTime) to 430 (sig.endTime), selects a price, verifies it with Binance, and gets a signature from Muon. They then call ForceActionsFacet::forceClosePosition() with the signature.

3. At block.timestamp = 502, although this bypasses the quote.deadline (500), the transaction will still be processed.

4. If we look at the first check:
   sig.endTime(430) + maLayout.forceCloseSecondCooldown(60) <= quote.deadline(500) -> (490 <= 500) ✅

5. If we look at the second check:
   sig.startTime(380) >= quote.statusModifyTimestamp(314) + maLayout.forceCloseFirstCooldown(60) -> (380 >= 374) ✅

6. The third check:
   sig.endTime(430) <= block.timestamp(502) - maLayout.forceCloseSecondCooldown(60) -> 430 <= 442 ✅

As a result, the transaction will execute and be processed. The purpose of showing this is to illustrate that it doesn't matter whether the quote's deadline has passed relative to the current block.timestamp. This is an intended design of the protocol to ensure that Party A doesn’t face any losses due to Party B's inaction. If there were a check between the block.timestamp and the deadline, it could lead to losses for Party A. Party A will try to close the position when they are in profit, and if Party B does not accept the request, Party A's only option would be to force close. If that fails due to the deadline, Party A would have to expire their position, open it again, and lose the opportunity to close the position at a time when they were in high profit. This could cause Party A to face a loss due to Party B's actions, which is why the protocol has not implemented such checks.

Root Cause

• PartyAFacet::expireQuote() can be called by anyone , so when the partyA status is close_pending , also quote.deadline < block.timestamp so partyB can call PartyAFacet::expireQuote() to stop partyA in force closing of the position as the partyB is in high loss.

Internal pre-conditions

1. partyB should act maliciously.(partyB can act malicious as it is a semi-trusted role. Also partyB is considered malicious in previous findings

External pre-conditions

No such condition's required

Attack Path

As given in Initialize.fixtures.ts, forceCloseFirstCooldown = 300 and forceCloseSecondCooldown = 120:

1. Party A has a quote with Party B, and Party A is in high profit while Party B is in high loss.

2. Party A calls PartyAFacet::requestToClosePosition() to close the request with a deadline 7 minutes (420 secs) from the current block.timestamp. Party A can give any deadline as there is no check for that.

   • quote.statusModifyTimestamp = block.timestamp = 300
   • quote.deadline = deadline = 720 (300 + 420)

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/PartyA/PartyAFacet.sol#L180-L182

3. Party B, in high loss, acts maliciously by not accepting the request, waiting for the prices to move in their favor before closing the position(partyB can act malicious as it is a semi-trusted role. Also partyB is considered malicious in previous findings).

4. At t = 500, Party A is still waiting for Party B to fill the close quote request. To call ForceActionsFacet::forceClosePosition(), sig.startTime() should be greater than quote.statusModifyTimestamp(300) + 300, and with currentTimestamp = 500, Party A is unable to call forceClosePosition() yet.

5. At t = 721 or later, Party A will be eligible to pass all the checks and will select a timeframe when the desired price was attained, verify it with Binance, get a signature from Muon, and call ForceActionsFacet::forceClosePosition() with the signature.

6. Before Party A can act, Party B front-runs the transaction and calls PartyAFacet::expireQuote() with the quoteId. Since the current block.timestamp(721) exceeds quote.deadline(720), the function executes and modifies the quote at the storage level. Party B could have called this function as soon as the condition quote.deadline < block.timestamp was met other then frontrunning it.

7. Party A's ForceActionsFacet::forceClosePosition() call will now fail since quote.quoteStatus = QuoteStatus.OPENED and quote.statusModifyTimestamp = block.timestamp.

8. Hence partyA will not be able to close his position when he wanted, and now he can't call ForceActionsFacet::forceClosePosition() with again that timeframe as quote.statusModifyTimestamp has changed with current block.timestamp. As a result, Party A cannot close the position as planned. Due to the volatile crypto market, Party A could now face losses while Party B profits from the market shift.

This vulnerability can also be exploited in other ways by Party B if Party A sets a shorter deadline. Party B could exploit the situation more easily since there are no checks on the deadline. Party A can set any value, and given that people often prefer quicker results, it's very likely that Party A might set a deadline of less than 5 minutes.

Impact

• Losses for Party A:
• Party A will not be able to close their position at their intended time.

PoC

No response

Mitigation

• Implement some checks while setting quote.deadline.
• Also add some modifier in expireQuote()