Zaros Part 1

Summary

OrderBranch::simulateTrade() performed a check using the wrong variable, causing the check to fail.

Vulnerability Details

1. The marginBalanceUsdX18 value returned according to the comment content. The account status before the simulated transaction should be used for calculation.

2. The position maintenance margin after the simulated transaction should be used as a parameter to call TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18).

3. When calling TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18) with marginBalanceUsdX18 as a parameter, orderFeeUsdX18 and settlementFeeUsdX18 should be calculated in advance, otherwise the marginBalanceUsdX18 of the simulated transaction will be greater than the value of the actual transaction.

Because this method is used in OrderBranch::createMarketOrder() to check funds.
We can assume a scenario:

1. The user creates a trading order for the first time, so the account is currently in the state: ctx.previousRequiredMaintenanceMarginUsdX18 == 0.

2. If you want to pass the check in the prevent liquidatable accounts from trading part of the code, you only need to ensure that marginBalanceUsdX18 > 0.

In summary, in the current case, the sizeDelta parameter when calling OrderBranch::simulateTrade() can be infinite.

We should consider using the requiredMaintenanceMarginUsdX18 after the simulated transaction and the marginBalanceUsdX18 before the transaction as parameters to execute the judgment in the if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18)){} part. And use the marginBalanceUsdX18 after deducting the fee

Poc

In order to facilitate viewing the data in the function, we add the test code in OrderBranch::simulateTrade() as follows:

Please add the test code to test/integration/perpetuals/order-branch/simulateTrade/simulateTrade.t.sol and execute

output:

Code Snippet

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

Impact

OrderBranch::simulateTrade() performed a check using the wrong variable, causing the check to fail.

Tools Used

Manual Review

Recommendations

Consider using the required maintenance margin after the trade and the margin balance before the trade for comparison

1. According to the annotation requirements, return the current margin balance of the account

2. The unrealized profit and loss of the account after the transaction will change slightly due to the slope. If the unrealized profit and loss becomes smaller as in the above test, the calculated account margin balance will be larger than the actual one. So the following uses the current margin balance and the required maintenance margin after the transaction for comparison

Note: The original function did not check whether the account is currently in a clear state, so the following modification content did not include related inspections. If necessary, please check at the beginning of the function.

For example:

test again, it's work
We can see in the test that simulateTrade() works as expected and the calculated post-trade margin balance is the same as the margin balance after the trade is completed.