cergyk - A party B can open a short position for a limit quote in a way which makes party A liquidatable

[sherlock-admin]

cergyk

high

A party B can open a short position for a limit quote in a way which makes party A liquidatable

Summary

A party B can open a position in a way which makes belonging party A liquidatable

Vulnerability Detail

We can see that a check is used before opening a position in order to ensure that opening a position makes neither A nor B liquidatable:
https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L150

However in the case of a SHORT limit position, it only ensures that unrealized pnl of the position does not make it unsolvent:
https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibSolvency.sol#L71C1-L82

In the case of a SHORT, the higher the opening price compared to actual price, the higher the positive pnl for A, so it should not revert here.

However we also see that the amount to lock is scaled accordingly in the opening position logic:
https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L162-L164

Which means that the higher opening price partyB chooses for the opening of the position, the higher the amount which will be locked by both parties as a collateral.

Even if the lockedAmount may be way larger than anticipated by partyA, this does not make it necessarily liquidatable because the position may be counter-balanced by the positive uPnL if actual price (oracle price) is lower than opening price:
https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/libraries/LibAccount.sol#L78-L86

The issue arises however in the case:

requestedOpenPrice << actualPrice
and actualPrice == openPrice

Since there is no restriction on the requestedOpeningPrice when sending a quote, two cases may arise:

• partyA intentionally creates a liquidatable position by choosing a very low requestedOpenPrice for a short quote.
• partyA creates a long lived quote and actual price drifts too high compared to requestedOpenPrice.

The solvency check should prevent A from being liquidated due to locking an amount too high.

Impact

partyA can be made to take a leverage too large during opening of short position by partyB and lose funds.

Code Snippet

Tool used

Manual Review

Recommendation

Adapt solvency check to take in account actual locked funds scaled by opening price

duplicate of #225

[MoonKnightDev]

same as issue #225

[SergeKireev]

Escalate

This is not a duplicate of #225, here we demonstrate that partyB can open a position in a way which makes partyA liquidatable, not himself.

This should be unique and high, since partyB causes a loss of funds to partyA with the possibility of profiting.

We will explain the detailed scenario which was mentioned in the initial report:

partyA creates a long lived quote and actual price drifts too high compared to requestedOpenPrice.

Detailed scenario:

symbolId: ETH

Market price for ETH is 2000$
partyA's allocated balance: 110

1. partyA sends quote:

{
    orderType: LIMIT,
    positionType: SHORT,
    requestedOpenPrice: 2000$,
    quantity: 1000
    cva: 50,
    lf: 50,
    mm: 10 
}

partyA allocated balance is 110, such as cva + lf + mm == allocated balance

2. Some time after quote is created, price moves to 2220$;

3. partyB opens a position to A's quote at price 2220$, such as upnl for both parties is 0;

• isSolventAfterOpenPosition doesn't revert in openQuote, since unscaled amounts are used (cva + lf + mm <= allocated balance)
• openedPrice > requestedPrice

Actual scaled locked amounts are 121 since (openedPrice/requestedOpenPrice = 1.11)

4. A is immediately liquidatable since:

function partyAAvailableBalanceForLiquidation(
    int256 upnl,
    address partyA
) internal view returns (int256) {
    AccountStorage.Layout storage accountLayout = AccountStorage.layout();
    int256 freeBalance = int256(accountLayout.allocatedBalances[partyA]) -
        int256(accountLayout.lockedBalances[partyA].cva + accountLayout.lockedBalances[partyA].lf);
    return freeBalance + upnl;
}

freeBalance = -1 and upnl = 0

Conclusion:

This is the only issue which outlines how a partyB can profit from causing immediate liquidation to a legitimate partyA, thus it should be unique and given the impact and likelihood of the issue, the severity should be high

[sherlock-admin2]

Escalate

This is not a duplicate of #225, here we demonstrate that partyB can open a position in a way which makes partyA liquidatable, not himself.

This should be unique and high, since partyB causes a loss of funds to partyA with the possibility of profiting.

We will explain the detailed scenario which was mentioned in the initial report:

partyA creates a long lived quote and actual price drifts too high compared to requestedOpenPrice.

Detailed scenario:

symbolId: ETH

Market price for ETH is 2000$
partyA's allocated balance: 110

1. partyA sends quote:

{
    orderType: LIMIT,
    positionType: SHORT,
    requestedOpenPrice: 2000$,
    quantity: 1000
    cva: 50,
    lf: 50,
    mm: 10 
}

partyA allocated balance is 110, such as cva + lf + mm == allocated balance

2. Some time after quote is created, price moves to 2220$;

3. partyB opens a position to A's quote at price 2220$, such as upnl for both parties is 0;

• isSolventAfterOpenPosition doesn't revert in openQuote, since unscaled amounts are used (cva + lf + mm <= allocated balance)
• openedPrice > requestedPrice

Actual scaled locked amounts are 121 since (openedPrice/requestedOpenPrice = 1.11)

4. A is immediately liquidatable since:

function partyAAvailableBalanceForLiquidation(
    int256 upnl,
    address partyA
) internal view returns (int256) {
    AccountStorage.Layout storage accountLayout = AccountStorage.layout();
    int256 freeBalance = int256(accountLayout.allocatedBalances[partyA]) -
        int256(accountLayout.lockedBalances[partyA].cva + accountLayout.lockedBalances[partyA].lf);
    return freeBalance + upnl;
}

freeBalance = -1 and upnl = 0

Conclusion:

This is the only issue which outlines how a partyB can profit from causing immediate liquidation to a legitimate partyA, thus it should be unique and given the impact and likelihood of the issue, the severity should be high

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.

[sherlock-admin2]

Escalate

This is an invalid issue. Please check #225 escalation

You've deleted an escalation for this issue.

[MoonKnightDev]

Escalate

This is not a duplicate of #225, here we demonstrate that partyB can open a position in a way which makes partyA liquidatable, not himself.

This should be unique and high, since partyB causes a loss of funds to partyA with the possibility of profiting.

We will explain the detailed scenario which was mentioned in the initial report:

partyA creates a long lived quote and actual price drifts too high compared to requestedOpenPrice.

Detailed scenario:

symbolId: ETH

Market price for ETH is 2000$ partyA's allocated balance: 110

1. partyA sends quote:

{
    orderType: LIMIT,
    positionType: SHORT,
    requestedOpenPrice: 2000$,
    quantity: 1000
    cva: 50,
    lf: 50,
    mm: 10 
}

partyA allocated balance is 110, such as cva + lf + mm == allocated balance

2. Some time after quote is created, price moves to 2220$;
3. partyB opens a position to A's quote at price 2220$, such as upnl for both parties is 0;

• isSolventAfterOpenPosition doesn't revert in openQuote, since unscaled amounts are used (cva + lf + mm <= allocated balance)
• openedPrice > requestedPrice

Actual scaled locked amounts are 121 since (openedPrice/requestedOpenPrice = 1.11)

4. A is immediately liquidatable since:

function partyAAvailableBalanceForLiquidation(
    int256 upnl,
    address partyA
) internal view returns (int256) {
    AccountStorage.Layout storage accountLayout = AccountStorage.layout();
    int256 freeBalance = int256(accountLayout.allocatedBalances[partyA]) -
        int256(accountLayout.lockedBalances[partyA].cva + accountLayout.lockedBalances[partyA].lf);
    return freeBalance + upnl;
}

freeBalance = -1 and upnl = 0

Conclusion:

This is the only issue which outlines how a partyB can profit from causing immediate liquidation to a legitimate partyA, thus it should be unique and given the impact and likelihood of the issue, the severity should be high

In Issue #225, PartyB also opens the short position at a price higher than what was requested, leading to liquidation. This appears to be the same bug. It's worth noting that if a user requests a position at an unfavorable price (higher than the market for LONG or lower for SHORT), they are effectively acknowledging the risk that PartyB might execute the position at that specified price.

[SergeKireev]

Escalate

This is not a duplicate of #225, here we demonstrate that partyB can open a position in a way which makes partyA liquidatable, not himself.

This should be unique and high, since partyB causes a loss of funds to partyA with the possibility of profiting.

We will explain the detailed scenario which was mentioned in the initial report:

partyA creates a long lived quote and actual price drifts too high compared to requestedOpenPrice.

Detailed scenario:

symbolId: ETH

Market price for ETH is 2000$ partyA's allocated balance: 110

1. partyA sends quote:

{
    orderType: LIMIT,
    positionType: SHORT,
    requestedOpenPrice: 2000$,
    quantity: 1000
    cva: 50,
    lf: 50,
    mm: 10 
}

partyA allocated balance is 110, such as cva + lf + mm == allocated balance

2. Some time after quote is created, price moves to 2220$;
3. partyB opens a position to A's quote at price 2220$, such as upnl for both parties is 0;

• isSolventAfterOpenPosition doesn't revert in openQuote, since unscaled amounts are used (cva + lf + mm <= allocated balance)
• openedPrice > requestedPrice

Actual scaled locked amounts are 121 since (openedPrice/requestedOpenPrice = 1.11)

4. A is immediately liquidatable since:

function partyAAvailableBalanceForLiquidation(
    int256 upnl,
    address partyA
) internal view returns (int256) {
    AccountStorage.Layout storage accountLayout = AccountStorage.layout();
    int256 freeBalance = int256(accountLayout.allocatedBalances[partyA]) -
        int256(accountLayout.lockedBalances[partyA].cva + accountLayout.lockedBalances[partyA].lf);
    return freeBalance + upnl;
}

freeBalance = -1 and upnl = 0

Conclusion:

This is the only issue which outlines how a partyB can profit from causing immediate liquidation to a legitimate partyA, thus it should be unique and given the impact and likelihood of the issue, the severity should be high

In Issue #225, PartyB also opens the short position at a price higher than what was requested, leading to liquidation. This appears to be the same bug. It's worth noting that if a user requests a position at an unfavorable price (higher than the market for LONG or lower for SHORT), they are effectively acknowledging the risk that PartyB might execute the position at that specified price.

In the scenario outlined above, partyA creates quote around market price and partyB opens the position at market price (which is different from partyA requested price and is not unfavorable to partyA, so this is just a scaling issue).

Issue #225 is handwavy about this and does not describe the concrete scenario

[Evert0x]

Although this report is describing the impact of the bug better, the root cause is the same as #225 .

[MoonKnightDev]

Fixed code PR link:
SYMM-IO/protocol-core#14

[hrishibhat]

Result:
Medium
Duplicate of #225
Agree with Evert's comments above. Maintaining the duplication.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• SergeKireev: rejected