Viktor_Cortess - Unrestricted access to forceCancelQuote(), forceCancelCloseRequest(), and forceClosePosition() Functions can cause system chaos

[sherlock-admin]

Viktor_Cortess

high

Unrestricted access to forceCancelQuote(), forceCancelCloseRequest(), and forceClosePosition() Functions can cause system chaos

Summary

The contract contains three functions responsible for forced operations, which are protected by only two modifiers: notLiquidated(quoteId) and whenNotPartyAActionsPaused.

 modifier notLiquidated(uint256 quoteId) {
    Quote storage quote = QuoteStorage.layout().quotes[quoteId];
    require(
        !MAStorage.layout().liquidationStatus[quote.partyA],
        "Accessibility: PartyA isn't solvent"
    );
    require(
        !MAStorage.layout().partyBLiquidationStatus[quote.partyB][quote.partyA],
        "Accessibility: PartyB isn't solvent"
    );
    require(
        quote.quoteStatus != QuoteStatus.LIQUIDATED && quote.quoteStatus != QuoteStatus.CLOSED,
        "Accessibility: Invalid state"
    );
    _;
}

modifier whenNotPartyAActionsPaused() {
    require(!GlobalAppStorage.layout().globalPaused, "Pausable: Global paused");
    require(!GlobalAppStorage.layout().partyAActionsPaused, "Pausable: PartyA actions paused");
    _;
}

This setup allows any malicious user to manipulate the system by closing quotes/positions or cancelling close requests.

Vulnerability Detail

The documentation lacks information about force operations and functions. However, in the Q&A section, it is mentioned that there are off-chain mechanisms like Liquidator Bots, Force close Bots, Force cancel Bots, and Anomaly detector Bots.

So the system should have force cancel/close Bots which a responsible for calling 3 functions mentioned above.

During the audit, it was discovered that the only preventive measure in place to restrict the use of these three functions is a require statement.

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

However, this require statement renders these three functions ineffective because the cooldown period is unreachable, as shown in this code:

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/control/ControlFacet.sol#L25-L27

On the other hand, the code includes three functions that set up cooldown periods, indicating that the developers plan to assign more realistic values to these periods.

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/control/ControlFacet.sol#L185-L206

Taking all factors into consideration, it can be concluded that these three force functions are within the scope of the audit and should have a modifier to protect them from being called by unauthorized users.

Impact

According to the readme file, the application will be deployed in multiple chains, including Arbitrum One, Arbitrum Nova, Fantom, Optimism, BNB chain, Polygon, and Avalanche. Transactions on these chains are relatively inexpensive, making it possible for a malicious user to create chaos in the system by canceling quotes and closing the positions of all users. This issue is classified as high severity because it has the potential to disrupt the entire protocol's functionality.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is recommended to add "Force close Bots" and "Force cancel Bots" modifiers to the forceCancelQuote(), forceCancelCloseRequest(), and forceClosePosition() functions to enhance their security and prevent unauthorized access.

[viktorcortess]

Escalate Force close functions should be called only by Force close Bots and Force cancel Bots mentioned in the documentation:

Q: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, input validation expectations, etc)?

Liquidator Bots, Force close Bots, Force cancel Bots, Anomaly detector Bots

I've just noticed in one of the issues a screenshot where one of the developers tells that PartyA calls forceClose function. Even in this case, a modifier onlyPartyAOfQuote(quoteId) should be added.

For example, function forceCancelQuote() can be called by anyone, so anyone can cancel any quote:

function forceCancelQuote(uint256 quoteId)
    external
    notLiquidated(quoteId)
    whenNotPartyAActionsPaused
{
    PartyAFacetImpl.forceCancelQuote(quoteId);
    emit ForceCancelQuote(quoteId, QuoteStatus.CANCELED);
}

It would be great to clarify with the developers the information about users who can invoke the force functions.

[sherlock-admin2]

Escalate Force close functions should be called only by Force close Bots and Force cancel Bots mentioned in the documentation:

Q: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, input validation expectations, etc)?

Liquidator Bots, Force close Bots, Force cancel Bots, Anomaly detector Bots

I've just noticed in one of the issues a screenshot where one of the developers tells that PartyA calls forceClose function. Even in this case, a modifier onlyPartyAOfQuote(quoteId) should be added.

For example, function forceCancelQuote() can be called by anyone, so anyone can cancel any quote:

function forceCancelQuote(uint256 quoteId)
    external
    notLiquidated(quoteId)
    whenNotPartyAActionsPaused
{
    PartyAFacetImpl.forceCancelQuote(quoteId);
    emit ForceCancelQuote(quoteId, QuoteStatus.CANCELED);
}

It would be great to clarify with the developers the information about users who can invoke the force functions.

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.

[ctf-sec]

 function forceCancelQuote(uint256 quoteId) internal {
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();
        MAStorage.Layout storage maLayout = MAStorage.layout();
        Quote storage quote = QuoteStorage.layout().quotes[quoteId];

        require(quote.quoteStatus == QuoteStatus.CANCEL_PENDING, "PartyAFacet: Invalid state");

take forceCancelQuote as an example

   require(quote.quoteStatus == QuoteStatus.CANCEL_PENDING, "PartyAFacet: Invalid state");

the force cancel quote cancel a quote in status CANCEL_PENDING

to set the status to CANCEL_PENDING

we need to call requestToCancelQuote which is guarded by the modifier onlyPartyAOfQuote

 function requestToCancelQuote(uint256 quoteId)
        external
        whenNotPartyAActionsPaused
        onlyPartyAOfQuote(quoteId)
        notLiquidated(quoteId)
    {

so the claim

anyone can cancel any quote:

is not true

[viktorcortess]

There are several possible statuses:

enum QuoteStatus {
PENDING, //0
LOCKED, //1
CANCEL_PENDING, //2
CANCELED, //3
OPENED, //4
CLOSE_PENDING, //5
CANCEL_CLOSE_PENDING, //6
CLOSED, //7
LIQUIDATED, //8
EXPIRED //9

}

Only PartyA can change the status to CANCEL_PENDING, but PartyB should accept this cancel Request to make the status CANCELED

function acceptCancelRequest(uint256 quoteId) internal {
    AccountStorage.Layout storage accountLayout = AccountStorage.layout();

    Quote storage quote = QuoteStorage.layout().quotes[quoteId];
    require(quote.quoteStatus == QuoteStatus.CANCEL_PENDING, "PartyBFacet: Invalid state");
    accountLayout.partyBNonces[quote.partyB][quote.partyA] += 1;
    quote.modifyTimestamp = block.timestamp;
    quote.quoteStatus = QuoteStatus.CANCELED;
    accountLayout.pendingLockedBalances[quote.partyA].subQuote(quote);
    accountLayout.partyBPendingLockedBalances[quote.partyB][quote.partyA].subQuote(quote);
    // send trading Fee back to partyA
    LibQuote.returnTradingFee(quoteId);

    LibQuote.removeFromPendingQuotes(quote);
}

With the current implementation, anybody can call force functions right in the next block after PartyA's request. So the idea of acceptance of requests by PartyB becomes pointless. Is it possible to invite sponsors to clarify the objectives of force functions and force cancel/close bots?

[ctf-sec]

Is it possible to invite sponsors to clarify the objectives of force functions and force cancel/close bots

sure @MoonKnightDev

[ctf-sec]

Only PartyA can change the status to CANCEL_PENDING, but PartyB should accept this cancel Request to make the status CANCELED

It is possible that PartyB never accept the cancel request to make the status cancelled then partyA or anyone can force cancel

[viktorcortess]

Only PartyA can change the status to CANCEL_PENDING, but PartyB should accept this cancel Request to make the status CANCELED

It is possible that PartyB never accept the cancel request to make the status cancelled then partyA or anyone can force cancel

Then accept and all force functions can be removed from the workflow. PartyA can simply close positions making status CANCELLED. That's why I'd like to clarify this moment with sponsors. Maybe there is an idea behind this triangle of finishing functions - request, force, and accept. And why any person can interfere in the relationships between PartyA and PartyB. In any case, this situation remains at the discretion of the judge.

[Evert0x]

With the current implementation, anybody can call force functions right in the next block after PartyA's request. So the > idea of acceptance of requests by PartyB becomes pointless. Is it possible to invite sponsors to clarify the objectives of force functions and force cancel/close bots?

@MoonKnightDev is this intended behavior?

[MoonKnightDev]

Only Party A can request to cancel a quote. If Party B has locked the quote, they must either accept the cancellation or open the position. If Party B doesn't respond, then any 3rd party has the authority to force-cancel it.

[hrishibhat]

Result: Invalid Unique Considering this a non issue since this is intended behaviour based on Sponsor's comment https://github.com/sherlock-audit/2023-06-symmetrical-judging/issues/312#issuecomment-1669354975

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• viktorcortess: rejected