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

liquidatable check in LiquidationBranch does not check for the liquidation fee

DPS

Summary

The function has a check to see if the current account should be liquidated. The problem is that it only checks the maintenance margin with the margin of the trader, it does not take into account the liquidation fee that the trader has to pay as well if he should be liquidated.

Vulnerability Details

This is where the check is performed

​
function liquidateAccounts(uint128[] calldata accountsIds) external {
// return if no input accounts to process
if (accountsIds.length == 0) return;
​
// fetch storage slot for global config
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
​
// only authorized liquidators are able to liquidate
if (!globalConfiguration.isLiquidatorEnabled[msg.sender]) {
revert Errors.LiquidatorNotRegistered(msg.sender);
}
​
// working data
LiquidationContext memory ctx;
​
>> // load liquidation fee from global config; will be passed in as `settlementFeeUsdX18`
>> // to `TradingAccount::deductAccountMargin`. The user being liquidated has to pay
>> // this liquidation fee as a "settlement fee"
>> ctx.liquidationFeeUsdX18 = ud60x18(globalConfiguration.liquidationFeeUsdX18);
​
// iterate over every account being liquidated; intentionally not caching
// length as reading from calldata is faster
for (uint256 i; i < accountsIds.length; i++) {
// store current accountId being liquidated in working data
ctx.tradingAccountId = accountsIds[i];
​
// sanity check for non-sensical accountId; should never be true
if (ctx.tradingAccountId == 0) continue;
​
// load account's leaf (data + functions)
TradingAccount.Data storage tradingAccount = TradingAccount.loadExisting(ctx.tradingAccountId);
​
// 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;
}
​
...
​

As we can see the liquidation fee that has to be paid by the trader if he is in liquidatable state is not being added to the check. This means that there will be cases where he has to be liquidated in order to have enough margin to cover his pnl and liquidation fee but he will not be because the isLiquidatable() will return false

Impact

High because a user's position will not be in liquidatable state when it should be. This might lead to cases where the protocol has to cover the liquidation fee because the user will not have enough margin deposited.

Tools Used

Manual review

Recommendations

Add the liquidation fee as well when checking if the trader's position is in liquidatable state

Updates

Lead Judging Commences

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

Liquidation doesn't take the liquidation fee in consideration inside the isLiquidatable check

Support

FAQs

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