xiaoming90 - DOS attack due to lack of penalty for `unlock`

[sherlock-admin]

xiaoming90

medium

DOS attack due to lack of penalty for unlock

Summary

Since there is no penalty for PartyB to lock and unlock a quote except for a temporary lock of their balance, this opens up an attack vector where a malicious PartyB could perform a denial-of-service (DOS) attack against PartyA, which negatively affects the protocol and its users.

Vulnerability Detail

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

File: PartyBFacetImpl.sol
22:     function lockQuote(uint256 quoteId, SingleUpnlSig memory upnlSig, bool increaseNonce) internal {
23:         QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
24:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
25: 
26:         Quote storage quote = quoteLayout.quotes[quoteId];
27:         LibMuon.verifyPartyBUpnl(upnlSig, msg.sender, quote.partyA);
28:         checkPartyBValidationToLockQuote(quoteId, upnlSig.upnl);
29:         if (increaseNonce) {
30:             accountLayout.partyBNonces[quote.partyB][quote.partyA] += 1;
31:         }
32:         quote.modifyTimestamp = block.timestamp;
33:         quote.quoteStatus = QuoteStatus.LOCKED;
34:         quote.partyB = msg.sender;
35:         // lock funds for partyB
36:         accountLayout.partyBPendingLockedBalances[msg.sender][quote.partyA].addQuote(quote);
37:         quoteLayout.partyBPendingQuotes[msg.sender][quote.partyA].push(quote.id);
38:     }

Once a user issues a quote, any PartyB can secure it by calling the lockQuote function, which will bar other PartyBs from interacting with the quote.

For any given reason, PartyB, having secured the quote, can choose to abandon the opening position by calling the unlockQuote function

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

File: PartyBFacetImpl.sol
40:     function unlockQuote(uint256 quoteId) internal returns (QuoteStatus) {
41:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
42: 
43:         Quote storage quote = QuoteStorage.layout().quotes[quoteId];
44:         require(quote.quoteStatus == QuoteStatus.LOCKED, "PartyBFacet: Invalid state");
45:         if (block.timestamp > quote.deadline) {
46:             QuoteStatus result = LibQuote.expireQuote(quoteId);
47:             return result;
48:         } else {
49:             accountLayout.partyBNonces[quote.partyB][quote.partyA] += 1;
50:             quote.modifyTimestamp = block.timestamp;
51:             quote.quoteStatus = QuoteStatus.PENDING;
52:             accountLayout.partyBPendingLockedBalances[quote.partyB][quote.partyA].subQuote(quote);
53:             LibQuote.removeFromPartyBPendingQuotes(quote);
54:             quote.partyB = address(0);
55:             return QuoteStatus.PENDING;
56:         }
57:     }

When a PartyB locks a quote, $x$ amount will be locked in its pending locked balance (partyBPendingLockedBalances). When the PartyB subsequently unlocks the quote, the same $x$ amount will be released from its pending locked balance.

Since there is no penalty for PartyB to lock and unlock a quote except for a temporary lock of their balance, this opens up an attack vector where a malicious PartyB could perform a denial-of-service (DOS) attack against PartyA. Whenever a PartyA creates a new quote, the malicious PartyB will step in and lock the quote but does not proceed to open the quote. PartyA could technically perform a force close against the locked quotes, but eventually, any new quotes created by the victim later will be locked by malicious PartyB too.

The whitelisting feature of the quote is not sufficient to guard against such an attack. If a PartyA wants its quote to be open for everyone for a valid reason, the PartyA cannot whitelist all the addresses in Ethereum except for the attacker address.

Since PartyA can only have a total of 15 pending quotes (maLayout.pendingQuotesValidLength) in their accounts, the victim will not be able to create new quotes if the attacker has locked all their existing quotes.

Another potential attack vector is that malicious PartyB could prevent other PartyBs from locking quotes. Whenever a PartyB attempt to lock a quote, the attacker would front-run them and lock the quote before them, and cause the victim's lock transaction to revert. The attacker will unlock the quote immediately after the attack to free up his pending locked balance.

Impact

Affected PartyA will be unable to create new quotes, and their existing pending quotes will be locked by the attacker who does not intend to open the positions. PartyB, who genuinely wants to lock+open a quote, will be unable to do so. These lead to a loss of opportunity cost for the affected PartyA and PartyB.

It also negatively affects the protocols as this issue could lead to fewer positions being opened, which in turn means less trading fee collected.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

To prevent malicious PartyB from abusing the lock+unlock functions, consider imposing some penalty/fee if PartyB decides to unlock the quote. For instance, the penalty/fee can be computed as a percentage of the locked quote's value, and the collected penalty/fee can be forwarded to the PartyA and/or protocol.

This measure will prevent abuse and encourage PartyB to think carefully before locking any position. When a PartyB locks a position but does not open it, it leads to a loss of opportunity cost for the quote's PartyA because other PartyB would have opened the position, and they would have already started profiting from the position. As such, it is fair for PartyA to charge a fee from PartyB to compensate for their loss.

[MoonKnightDev]

We currently don't apply any penalty for unlocking. This is because party B may lock one limit position at a time and when they attempt to open it, they may find that the user is no longer solvent. In such a case, party B is not acting maliciously, but needs to unlock it.

Now, regarding the DOS attack prevention methods you mentioned, we have the following measures in place:

1- Users have the option not to include potentially malicious party B in their whitelisted hedgers.

2- The protocol can identify and deregister party Bs who consistently engage in this behaviour and are deemed malicious. In fact, we might consider imposing penalties on these party Bs, particularly if they have placed a collateral stake with us outside of the contract.

[snn20]

escalate for 10 The warden points out that the impact is maybe loss of trading fees and opportunity cost This isn't a loss of funds as Sherlock's docs maintain for medium severity

here is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant)

I don't think opportunity cost should be counted as a loss of funds

[sherlock-admin2]

escalate for 10 The warden points out that the impact is maybe loss of trading fees and opportunity cost This isn't a loss of funds as Sherlock's docs maintain for medium severity

here is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant)

I don't think opportunity cost should be counted as a loss of funds

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.

[xiaoming9090]

Escalate

escalate for 10 The warden points out that the impact is maybe loss of trading fees and opportunity cost This isn't a loss of funds as Sherlock's docs maintain for medium severity

here is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant)

I don't think opportunity cost should be counted as a loss of funds

This protocol is intended for retail traders and well-known trading entities. Having their investing funds unnecessarily locked up would inevitably cause them to suffer a loss. Besides that, this issue also leads to a loss of trading fees for the protocol.

Regarding the sponsor's comment:

Now, regarding the DOS attack prevention methods you mentioned, we have the following measures in place:

1- Users have the option not to include potentially malicious party B in their whitelisted hedgers.

2- The protocol can identify and deregister party Bs who consistently engage in this behaviour and are deemed malicious. In fact, we might consider imposing penalties on these party Bs, particularly if they have placed a collateral stake with us outside of the contract.

For point 1, if a user wants their position to be opened by any PartyB except for the malicious PartyB, there is no way to configure this in the current system. In this case, the user has to whitelist all the PartyB in the systems, which can be a very large number if the protocol grows or become popular, which is infeasible. It will only work if the system also has a blacklist mechanism, which was not found in the current system.

For point 2, I agree that these measures will significantly reduce the chance of this issue happening. However, as far as I know, in the context of this audit, this anomaly detection mechanism to identify malicious behaviors has not been implemented or is ready to be deployed.

[sherlock-admin2]

Escalate

escalate for 10 The warden points out that the impact is maybe loss of trading fees and opportunity cost This isn't a loss of funds as Sherlock's docs maintain for medium severity

here is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant)

I don't think opportunity cost should be counted as a loss of funds

This protocol is intended for retail traders and well-known trading entities. Having their investing funds unnecessarily locked up would inevitably cause them to suffer a loss. Besides that, this issue also leads to a loss of trading fees for the protocol.

Regarding the sponsor's comment:

Now, regarding the DOS attack prevention methods you mentioned, we have the following measures in place:

1- Users have the option not to include potentially malicious party B in their whitelisted hedgers.

2- The protocol can identify and deregister party Bs who consistently engage in this behaviour and are deemed malicious. In fact, we might consider imposing penalties on these party Bs, particularly if they have placed a collateral stake with us outside of the contract.

For point 1, if a user wants their position to be opened by any PartyB except for the malicious PartyB, there is no way to configure this in the current system. In this case, the user has to whitelist all the PartyB in the systems, which can be a very large number if the protocol grows or become popular, which is infeasible. It will only work if the system also has a blacklist mechanism, which was not found in the current system.

For point 2, I agree that these measures will significantly reduce the chance of this issue happening. However, as far as I know, in the context of this audit, this anomaly detection mechanism to identify malicious behaviors has not been implemented or is ready to be deployed.

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.

[KuTuGu]

Escalate The partA user can call requestToCancelQuote && forceCancelQuote and simply wait for forceCancelCooldown to cancel the lock and DOS does not result in a loss of funds. Judging criteria based on sherlock, the short DOS is invalid:

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? It would not count if the DOS, etc. lasts a known, finite amount of time <1 year. If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation. The greater the cost of the attack for an attacker, the less severe the issue becomes.

[sherlock-admin2]

Escalate The partA user can call requestToCancelQuote && forceCancelQuote and simply wait for forceCancelCooldown to cancel the lock and DOS does not result in a loss of funds. Judging criteria based on sherlock, the short DOS is invalid:

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? It would not count if the DOS, etc. lasts a known, finite amount of time <1 year. If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation. The greater the cost of the attack for an attacker, the less severe the issue becomes.

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.

[JeffCX]

Since there is no penalty for PartyB to lock and unlock a quote except for a temporary lock of their balance, this opens up an attack vector where a malicious PartyB could perform a denial-of-service (DOS) attack against PartyA. Whenever a PartyA creates a new quote, the malicious PartyB will step in and lock the quote but does not proceed to open the quote. PartyA could technically perform a force close against the locked quotes, but eventually, any new quotes created by the victim later will be locked by malicious PartyB too.

This steps can be repeated to perform denial of services and consider the trading is a very time sensitive action,

I have to agree with senior watson's comment and escalation

[kiseln]

This steps can be repeated to perform denial of services and consider the trading is a very time sensitive action,

I have to agree with senior watson's comment and escalation

PartyA will remove malicious PartyB from a list of hedgers that can accept their quotes so I disagree that this is repeatable.

[xiaoming9090]

PartyA will remove malicious PartyB from a list of hedgers that can accept their quotes so I disagree that this is repeatable.

As I have already mentioned in my early response here:

If a user wants their position opened by any PartyB except for the malicious PartyB, there is no way to configure this in the current system. In this case, the user has to whitelist all the PartyB in the systems, which can be a very large number if the protocol grows or become popular, which is infeasible. It will only work if the system also has a blacklist mechanism, which was not found in the current system.

[kiseln]

Adding a quote from the sponsor that this is an intended mechanism. Protocol is supposed to create a competitive environment for hedgers which disincentivizes malicious behavior

[xiaoming9090]

Adding a quote from the sponsor that this is an intended mechanism. Protocol is supposed to create a competitive environment for hedgers which disincentivizes malicious behavior

PartyB will be incentivized to behave properly. However, that does not mean there we do not need to put in place measures to manage the PartyB. In an event where the benefit outweighs the risk of losing a customer, malicious PartyB will choose to carry out the attack.

[Evert0x]

Seems like a DOS attack can be performed when PartyA is not handling a whitelist and/or PartyB is switching between addresses to keep locking the quote

[naveedinno]

As PartyBs are restricted in the current version, there won't be any problems of this kind, and thus, we don't consider it to be an issue

[hrishibhat]

Result: Low Has duplicates After further internal discussion and additional points raised by the Sponsor, reverting the previous decision and considering this a valid low. Sponsor comments:

its very unlikely for a user to be interested in send a quote that should be answered by all hedgers he should use frontends to help him select the best hedgers or use driver systems like used in cowswap as making it "open for all" just incentivizes the fastest hedger and not the one who has the best offer

Lead Watson comment:

In that case, I would agree that having a whitelisting mechanism will be sufficient in mitigating this issue since real-world usage does not really expect someone to send a quote/position to be answered by all hedgers. Users are expected to use the tools (e.g., front-end, driver system) to identify the best hedgers before opening the quote and adding them to the whitelisting.

DOS in this case does not have a severe impact as a user can re-open a position with another hedger, hence This would be the intended design and users must whitelist the hedgers accordingly.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• snn20: accepted
• kutugu: accepted
• xiaoming9090: rejected