Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Incorrect Liquidation Check while creating order

cryptomoon

Summary

In OrderBranch, function simulateTradehas incorrect liquidation.

Vulnerability Details

function simulateTradeis called by function createMarketOrder. The function simulateTradehas checks which ensure that the creation of market order will fail if tradingAccountis liquidatable after the ordersettlement. However, the condition being used is wrong.

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

(requiredInitialMarginUsdX18, requiredMaintenanceMarginUsdX18, ctx.accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(marketId, ctx.sizeDeltaX18);
​
// use unrealized PNL to calculate & output account's margin balance
marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(ctx.accountTotalUnrealizedPnlUsdX18);
{
// get account's current required margin maintenance (before this trade)
(, ctx.previousRequiredMaintenanceMarginUsdX18,) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
// prevent liquidatable accounts from trading
if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
revert Errors.AccountIsLiquidatable(tradingAccountId);
}
}

The check is used to ensure that previousRequiredMaintenanceMarginUsdX18(before order settlement) is less than marginBalanceUsdX18to prevent liquidatable accounts from trading as mentioned in comments. However, marginBalanceUsdX18is calculated considering the sizeDeltaonce the order is settled. This is not correct way because it should calculate marginBalanceUsdX18only considering current state of tradingAccountand not based on sizeDeltaof created order.

Impact

marginBalanceUsdX18calculated may not be accurate because it considers the sizeDeltaand not current state of account.

Tools Used

Manual

Recommendations

The correct code is as follows(added the comments where changes is made):

(requiredInitialMarginUsdX18, requiredMaintenanceMarginUsdX18, ctx.accountTotalUnrealizedPnlUsdX18) =\
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(marketId, ctx.sizeDeltaX18);
// use unrealized PNL to calculate & output account's margin balance
{
//****************************CHANGES MADE HERE*****************************************
(, ctx.previousRequiredMaintenanceMarginUsdX18, accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
//****************************CHANGES MADE HERE*****************************************
marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
​
​
// prevent liquidatable accounts from trading
if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
revert Errors.AccountIsLiquidatable(tradingAccountId);
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

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

The protocol compares the margin balance after the trade with current required maintenance margin to determine if the trading account is currently liquidatable

Support

FAQs

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

Give us feedback!