DeFiFoundry
60,000 USDC
View results
Submission Details
Severity: medium
Valid

Executing a TP/SL order in one market and thereby increasing the nonce of the account cancels all offchainOrders in all other markets

Lines of code

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L231-L238

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L296-L301

Impact

Because an account has only one nonce for all markets, executing a TP/SL offChainOrder and thereby increasing the accounts nonce will cancel all offchain orders for any other market. This can lead to significant losses for the user since profit taking and stop losses for the other markets will thereby be disabled.

Proof of Concept

In the fillOffchainOrders() function, the system verifies an offChainOrder by comparing the nonce of the offChainOrder with the current nonce of the account:

if (ctx.offchainOrder.nonce != tradingAccount.nonce) {
revert Errors.InvalidSignedNonce(tradingAccount.nonce, ctx.offchainOrder.nonce);
}

Since not every offchainOrder increases the accounts nonce, this allows a user to sign multiple offChainOrders which can all be executed. But as stated in the netspec,

TP/SL must increase the nonce in order to prevent older limit orders from being filled.

The issue arises from the fact that the account only has one nonce for all markets which means that if a TP/SL is executed in a market and increases the nonce of the account, all other offchain orders will no longer be executable. This also includes any TP/SL order in any other market which were placed to protect the position in another market or take some profit.

Example:

  1. Alice has an active offchain order in Market A and Market B. Both orders are stop loss orders to limit the loss Alice accrues

  2. The crypto market falls and Alice´s SL order for market A is executed which checks the nonce.

  3. The nonce is valid, and the order is filled, but this action increments the nonce for Alice's account.

  4. As a result, the SL orders in Market B can no longer be executed since it was signed for the previous nonce

  5. If the asset price for Market B falls further, Alice’s position is no longer protected by the SL order since it can no longer be executed leading to financial loss for Alice

Recommended Mitigation Steps

To avoid the scenario described above consider adding individual nonces for each market. This can be done by adding a mapping to the userAccount data which maps the market address to its individual nonce which is used for checking the validity of an offchain order.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

fillOffchainOrders reverts everything if a single order fails one of the multiple checks

If you send 1 cancel and 1 create it should still run the cancel, not revert everything.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.