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

Incorrect liquidatable checking for market order creation

h2134

Summary

Incorrect marginBalanceUsdX18 is used for the liquidatable checking during market order creation, protocol may wrongly prevent an order from creating.

Vulnerability Details

When a user calls to create a market order, protocol will simulate the settlement costs and validity of a given order. During the simulation, protocol checks if the current trading account is liquidatable and prevents liquidatable accounts from trading.

// calculate & output required initial & maintenance margin for the simulated trade
// and account's unrealized PNL
(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);
}
}

As we can see from above, the account's current required margin maintenance is compared to marginBalanceUsdX18 to determine if the account is liquidatable.

// prevent liquidatable accounts from trading
if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
revert Errors.AccountIsLiquidatable(tradingAccountId);
}

The marginBalanceUsdX18 is calculated based on ctx.accountTotalUnrealizedPnlUsdX18, which is calculated by getAccountMarginRequirementUsdAndUnrealizedPnlUsd() in TradingAccount lib.

// calculate & output required initial & maintenance margin for the simulated trade
// and account's unrealized PNL
(requiredInitialMarginUsdX18, requiredMaintenanceMarginUsdX18, ctx.accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(marketId, ctx.sizeDeltaX18);
​
​
// use unrealized PNL to calculate & output account's margin balance
marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(ctx.accountTotalUnrealizedPnlUsdX18);

getAccountMarginRequirementUsdAndUnrealizedPnlUsd() is called with marketId and ctx.sizeDeltaX18 arguments, in this function, ctx.sizeDeltaX18 is used for calculating markPrice which in turn is used to get accountTotalUnrealizedPnlUsdX18.

// calculate price impact of the change in position
UD60x18 markPrice = perpMarket.getMarkPrice(sizeDeltaX18, perpMarket.getIndexPrice());
​
...
​
// get unrealized pnl + accrued funding fees
SD59x18 positionUnrealizedPnl =
position.getUnrealizedPnl(markPrice).add(position.getAccruedFunding(fundingFeePerUnit));

By doing that, the accountTotalUnrealizedPnlUsdX18 value returned is essentially the value after the trade, hence the marginBalanceUsdX18 is not the current margin balance but the margin balance after the trade. It's obviously wrong that protocol compares the margin balance after the trade with current required maintenance margin to determine if the trading account is currently liquidatable.

In fact, when a trading account is actually being liquidated, protocol compares the trading account's current margin balance with current required margin maintenance to see if the account is liquidatable, and that is the correct checking.

// get account's required maintenance margin & unrealized PNL
(, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =
@> tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
​
// get then save margin balance into working data
ctx.marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
​
​
// if account is not liquidatable, skip to next account
// account is liquidatable if requiredMaintenanceMarginUsdX18 > ctx.marginBalanceUsdX18
if (!TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, ctx.marginBalanceUsdX18)) {
continue;
}

Impact

If the skew is to be decresed after the trade, mark price becomes lower hence lower the accountTotalUnrealizedPnlUsdX18, leads to a lower margin balance than the current margin balance, a trader might be wrongly prevented from creating the order, under certain circumstances, the impact could be critical as the position continues to deteriorate and goes below the maintenance margin requirement which would cause their position to be liquidated

Tools Used

Manual Review

Recommendations

Should use the trading account's current margin balance to check against **current required margin maintenance **to see if the account is liquidatable.

{
// get account's current required margin maintenance (before this trade)
- (, ctx.previousRequiredMaintenanceMarginUsdX18,) =
- tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
+ (, ctx.previousRequiredMaintenanceMarginUsdX18, ctx.accountTotalUnrealizedPnlUsdX18) =
+ tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
+ marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(ctx.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
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!