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

Missing `isLiquidatable` check in `_fillOrder()`

tigerfrake

Summary

An account isLiquidatable if the account's current required margin maintenance (before this trade) i.e (previousRequiredMaintenanceMarginUsdX18) is greater than the account's margin balance i.e (marginBalanceUsdX18).
During createMarketOrder(), this is checked in simulateTrade() to prevent liquidatable accounts from trading.

However, in _fillOrder(), this is not checked. The issue here is that there is a possiblity that the window between the time when an order was created and the current time when it is being filled, prices may have flactuated and therefore marginBalanceUsdX18 may have changed with repect to previousRequiredMaintenanceMarginUsdX18.

Vulnerability Details

createMarketOrder() invokes simulateTrade() which performs the following check:

// 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);
}
}

Notice that it uses unrealized PNL to calculate & output account's margin balance(marginBalanceUsdX18) by calling tradingAccount.getMarginBalanceUsd():

// iterate over every collateral account has deposited
for (uint256 i; i < cachedMarginCollateralBalanceLength; i++) {
// read key/value from storage for current iteration
(address collateralType, uint256 balance) = self.marginCollateralBalanceX18.at(i);
​
// load collateral margin config for this collateral type
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// calculate the collateral's "effective" balance as:
// collateral_price * deposited_balance * collateral_loan_to_value_ratio
>> UD60x18 adjustedBalanceUsdX18 = marginCollateralConfiguration.getPrice().mul(ud60x18(balance)).mul(
ud60x18(marginCollateralConfiguration.loanToValue)
);
​
// add this account's "effective" collateral balance to cumulative output
marginBalanceUsdX18 = marginBalanceUsdX18.add(adjustedBalanceUsdX18.intoSD59x18());
}
​
// finally add the unrealized PNL to the cumulative output
>> marginBalanceUsdX18 = marginBalanceUsdX18.add(activePositionsUnrealizedPnlUsdX18);

The getMarginBalanceUsd() function iterates over every collateral the account has deposited and calls getPrice() to calculate the collateral's "effective" balance and finally returns the margin balance of the account in usd.
Now, getPrice() returns the price of the given margin collateral type by calling ChainlinkUtil.getPrice(). With such oracle query, prices always fluctuate.

The problem however lies in how this whole process works.

  1. A user creates an order at time t. During this time, previousRequiredMaintenanceMarginUsdX18 = x and marginBalanceUsdX18 = x+2

  2. At time t+3 the filler tries to fill this order. However, the prices have flactuated since the time this order was created.
    At this time of filling order, previousRequiredMaintenanceMarginUsdX18 = x+1 and marginBalanceUsdX18 = x. Therefore, this account isLiquidatable.

However, in SettlementBranch:_fillOrder(), this check is not performed at all. The function assumes that once the isLiquidatable state is checked during order creation, it will remain the same throughout which is not the case. Note that one account can have multiple open positions in different markets and all these positions rely on the users margin balance.

There is a chance that these position may be in losses during this time of filling an order meaning that the remaining marginBalanceUsdX18 could have dropped with respect to previousRequiredMaintenanceMarginUsdX18.

In createMarketOrder() function, the following two checks are done in order:

  1. Check that account's margin balance is greater than current required margin maintenance (before this trade) via TradingAccount.isLiquidatable() in simulateTrade()

  2. Check that the trader can satisfy the appropriate margin requirement via tradingAccount.validateMarginRequirement()

In SettlementBranch:fillMarketOrder() however, only the second check is done here.

Impact

Without checking the liquidation status, liquidatable accounts might engage in trades which according to protocol, should not be allowed. Such accounts should be liquidated.

Tools Used

Manual Review

Recommendations

Perform a check to ensure that liquidatable accounts are prevented from trading:
In _fillOrder(), add this check:

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

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`isLiquidatable` check missing in `_fillOrder()`

Support

FAQs

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