search

sherlock-audit / 2024-09-symmio-v0-8-4-update-contest-judging

0 stars 0 forks source link

safdie - Lack of `require` for quote validity in `lockQuote` allow attackers to attempt to lock already-locked quotes or expired quotes in `PartyBQuoteActionsFacetImpl.sol` #19

Open sherlock-admin3 opened 1 week ago

sherlock-admin3 commented 1 week ago

safdie

High

Lack of require for quote validity in lockQuote allow attackers to attempt to lock already-locked quotes or expired quotes in PartyBQuoteActionsFacetImpl.sol

Summary

The lockQuote function does not check if the quote is in a valid state before locking (e.g., verifying that the quote is not already locked or in an invalid state). This could allow attackers to attempt to lock already-locked quotes or expired quotes.

Root Cause

The lockQuote function in PartyBQuoteActionsFacetImpl.sol contract is a crucial point for ensuring the integrity of the system by validating quotes before they are locked. A lack of require statements for quote validity can lead to several vulnerabilities. https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/PartyBQuoteActions/PartyBQuoteActionsFacetImpl.sol#L23-L31

  1. Missing quote state check: There are no checks to ensure that the quote is in a valid state for locking. Quotes should typically be in a specific status (e.g., OPEN, CLOSE_PENDING, etc.) before they can be locked. This could allow a user to lock a quote that is not valid for such an operation. If a quote is in an invalid state (like CANCELED or EXPIRED), locking it could lead to inconsistent states and potential loss of funds or erroneous behavior in the system. 
    require(quote.quoteStatus == QuoteStatus.LOCKED || quote.quoteStatus == QuoteStatus.PENDING, "Quote is not in a valid state to lock");
  2. Quote existence: There should be a check to confirm that the quote with the specified quoteId actually exists. If an attacker attempts to lock a quote that does not exist (e.g., an out-of-bounds ID), it could lead to unexpected behaviors. 
    require(quoteId < quoteLayout.quotes.length, "Quote does not exist");
  3. Ownership/permissions check: It might be necessary to check if the caller (msg.sender) is authorized to lock this specific quote. If there’s a lack of permission checks, any party could lock quotes that don’t belong to them, leading to potential misuse. 
    require(msg.sender == quote.partyB, "Not authorized to lock this quote");
  4. Quote expiry check: If quotes have a specific deadline, you should check whether the current time exceeds that deadline before allowing a lock operation. 
    require(block.timestamp <= quote.deadline, "Quote has expired");

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

  1. Users could lock quotes that should not be lockable, leading to a mishandling of state and potential fund loss.
  2. Locking an expired or canceled quote could result in unwanted states that the system does not account for.
  3. Without permission checks, users could manipulate quotes that do not belong to them.

PoC

No response

Mitigation

Add the aforementioned require statements to ensure the quote is valid, exists, and the caller has the correct permissions.