Zaros Part 1

Summary

The simulateTrade function of OrderBranch.sol is used to check in advance if the market position will become liquidatable after the order gets executed. However it incorrectly compares the balances (after the trade) against the margin requirements(before the trade) - which leads to imprecise validation.

Vulnerability Details

The simulateTrade function of OrderBranch.sol simulates market order execution using current market conditions to check if the order can be filled, without violating the required collateral thresholds. This is how the most vital part of the check looks like:

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L123-L137

This is the order of operation in the above snippet:

• tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd() is called with the size of the trade so that it can simulate the value of accountTotalUnrealizedPnlUsdX18 after the execution of the order

• after that marginBalanceUsdX18 is calculated, using accountTotalUnrealizedPnlUsdX18 AFTER the trade

• another block of code follows that calls again tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd() , however this time with 0 values, meaning that the calculated values will be the current ones and the changes after order execution are NOT reflected

• finally a isLiquidatable check is made on the account, which compares if marginBalanceUsdX18 (AFTER the trade) is lower than the maintenance margin (BEFORE the trade)

The problem is in the last step, because the isLiquidatable check is conducted between values from 2 different stages, instead of the same stage.

Example:

• Let’s assume:

  • the calculated price of collateral is 1$ to keep it simple

  • maintenanceMarginRate is 1.2

  • marginBalanceUsd without PnL is 1500$

  • the simulated order reduces the position size from 1000 to 500 tokens

  • based on the above the calculated requiredMaintenanceMargin (AFTER the trade) = (position size * price * maintenanceMarginRate )⇒ 500 * 1$ * 1.2 = 600$

  • the PnL (AFTER the trade) due to the price change is reduced in half from 800$ to 400$ . This means that the calculated marginBalance (AFTER the trade) is 1500$ - 400$ ⇒ 1100$

  • previousRequiredMaintenanceMargin (BEFORE the trade) = (position size * price * maintenanceMarginRate )⇒ 1000 * 1$ * 1.2 = 1200$

  • isLiquidatable() checks that previousRequiredMaintenanceMargin > marginBalance ( 1200$ > 1100$) ⇒ which returns true and respectively reverts the function, because the simulation concludes that after the trade the position will not be healthy anymore

  As you can see calculating requiredMaintenanceMargin and marginBalanceUsd from 2 different stages produces inaccurate results. If the proper requiredMaintenanceMargin (AFTER the trade) was used the the calculation would succeed ( because (600$ is not > 1100$) and the call will not revert and market order creation will not be prevented.

Impact

Market order creation will be inaccurately reverted and will prevent healthy accounts from trading

Tools Used

Manual Review

Recommended Mitigation

Update the simulateTrade() function so that it properly to compares the values from the same stage: