safdie - Arbitrary withdrawal/balance manipulation in `acceptCancelRequest` will lead to incorrect balance deductions or unintended refund in `PartyBQuoteActionsFacetImpl.sol `

[sherlock-admin3]

safdie

Medium

Arbitrary withdrawal/balance manipulation in acceptCancelRequest will lead to incorrect balance deductions or unintended refund in PartyBQuoteActionsFacetImpl.sol

Summary

In the acceptCancelRequest function, the contract subtracts the quote’s locked balance from accountLayout.pendingLockedBalances and refunds the trading fee to partyA. However, if the quote is in an inconsistent state or the refund logic is flawed, this could lead to incorrect balance deductions or unintended refunds.

Root Cause

The vulnerability in the acceptCancelRequest function allows manipulation of the balance for Party A or Party B when canceling a quote. This is due to the improper validation of the state and amounts when canceling a quote. Specifically, if a malicious actor can manipulate the quote object or its associated storage, they could inflate or deflate the balances of partyA or partyB.

The flaw arises in the following segment: https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/PartyBQuoteActions/PartyBQuoteActionsFacetImpl.sol#L59-L64 Here, the balances are subtracted using the subQuote() function, which not have sufficient validation or safeguards in place to ensure the consistency of the quote's state. If an attacker can manipulate the quote's attributes (such as inflating the lockedValues), they can cause a miscalculation in the balance deduction.

The function subQuote() is a helper that calls another internal function sub() to subtract the locked values of a Quote from a LockedValues struct. https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/libraries/LibLockedValues.sol#L64-L66 In this case, subQuote() just takes self, which is the LockedValues struct, and subtracts the values inside quote.lockedValues. This makes the security of this operation depend on:

1. Proper validation of quote.lockedValues: If the values in quote.lockedValues can be inflated or manipulated before calling subQuote(), the subtraction will be based on incorrect or malicious data.
2. Integrity of sub(): The security also relies on how the sub() function handles subtraction.

The sub() function indicates a direct subtraction of values from a LockedValues struct. This implementation vulnerable to arbitrary withdrawal or balance manipulation in the acceptCancelRequest(). Let’s analyze this in detail: https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/libraries/LibLockedValues.sol#L50-L56 So, if quote.lockedValues (used in subQuote() which calls sub()) can be manipulated by an attacker, they could make the subtraction subtract more than intended, or even potentially negative values, depending on how those locked values are managed. Also, the line accountLayout.allocatedBalances[quote.partyA] += fee; adds a fee back to Party A's balance without sufficient checks to ensure that the quote is valid, or that the withdrawal is legitimate. If an attacker can manipulate the conditions that lead to acceptCancelRequest(), they could request an invalid quote cancellation and receive unexpected funds due to the fee being improperly accounted.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. A malicious actor (Party B) sets up a quote with inflated locked values or manipulates the quote object through some vulnerability or input bug. The goal is to create a situation where the locked values exceed the actual balance or fee involved in the trade.

   // Example of creating a quote with inflated locked values
   quote.lockedValues = LockedValues({ 
   partyA: 1000 ether, // Fake a very high amount for manipulation
   partyB: 500 ether
   });

2. The attacker triggers acceptCancelRequest() with the malicious quote. This will cause the contract to subtract the inflated lockedValues from the pendingLockedBalances of both Party A and Party B. Since the values are artificially inflated, the contract will incorrectly subtract these large amounts from their balances.

   // Party B triggers the cancellation of the inflated quote
   partyBContract.acceptCancelRequest(quoteId);

3. The subQuote() function will reduce the pendingLockedBalances using the inflated lockedValues. Meanwhile, the refund logic (accountLayout.allocatedBalances[quote.partyA] += fee) will still refund the fee to Party A based on the original, legitimate trade, not the inflated values. The attacker could:
   • Inflate the locked balance to manipulate the subQuote() operation.
   • Receive an incorrect refund due to inconsistencies between the real fee and inflated locked value.

Impact

1. If Party B artificially inflates the lockedValues, the subQuote() function will deduct a larger-than-expected amount from Party A's pendingLockedBalances, potentially depleting their balance or locking more funds than intended. This could lead to Party A losing a significant amount of funds if the vulnerability is exploited in this way.
2. The trading fee is refunded to Party A after canceling the quote. If the locked values are manipulated, Party A may receive an incorrect fee refund (too small or too large), leading to potential financial discrepancies.
3. If the system allows Party B to manipulate the pendingLockedBalances, Party B could receive incorrect or excessive deductions from their balance, allowing them to withdraw funds that should have been locked for other trades.

PoC

No response

Mitigation

1. Ensure that the subQuote() function is thoroughly validated and checks for consistent quote states, including ensuring that the locked values are within a valid range.
2. Instead of relying on the stored values for locked balances and fees, recompute them based on current data at the time of quote cancellation. This avoids relying on potentially manipulated or outdated storage values.
3. Emit additional events that track changes in balances and refunds, allowing for easier detection of anomalies in the quote cancellation process.