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

Liquidations could revert

samuraii77

Summary

Liquidations could revert

Vulnerability Details

Whenever a user is getting liquidated, the following function is called:

ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
pnlUsdX18: requiredMaintenanceMarginUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});

This is a part of the function being called:

if (settlementFeeUsdX18.gt(UD60x18_ZERO) && ctx.settlementFeeDeductedUsdX18.lt(settlementFeeUsdX18)) {
// attempt to deduct from this collateral difference between settlement fee
// and amount of settlement fee paid so far
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
settlementFeeUsdX18.sub(ctx.settlementFeeDeductedUsdX18),
feeRecipients.settlementFeeRecipient
);
​
// update amount of settlement fee paid so far by the amount
// that was actually withdraw from this collateral
ctx.settlementFeeDeductedUsdX18 = ctx.settlementFeeDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// order fee logic same as settlement fee above
if (orderFeeUsdX18.gt(UD60x18_ZERO) && ctx.orderFeeDeductedUsdX18.lt(orderFeeUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
orderFeeUsdX18.sub(ctx.orderFeeDeductedUsdX18),
feeRecipients.orderFeeRecipient
);
ctx.orderFeeDeductedUsdX18 = ctx.orderFeeDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// pnl logic same as settlement & order fee above
if (pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(pnlUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
feeRecipients.marginCollateralRecipient
);
ctx.pnlDeductedUsdX18 = ctx.pnlDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// if there is no missing margin then exit the loop
// since all amounts have been deducted
if (!ctx.isMissingMargin) {
break;
}

We can see that we go through the different payments the user has to make, in this case all of his collateral tokens and a liquidation fee. Due to the code, if the user has 2 different collateral tokens deposited with 50 tokens of each and he has to pay 100 tokens, the function will first go through the first collateral, take the tokens and then get the rest from the second collateral token.

Let's imagine the following scenario:

  1. Bob is getting liquidated, he has 100 tokens to pay as the PnL parameter and 50 tokens as liquidation fees

  2. He has 50 tokens of his first collateral token and 50 of his second one

  3. The function first takes his 50 tokens as liquidation fees from the first collateral token and he is left with no amount of it anymore

  4. The function still continues running and we end up in the PnL check (where he has to pay 100 tokens) despite him having no tokens

  5. We end up in this else check as he doesn't have enough tokens for the payment:

    else {
    withdraw(self, collateralType, marginCollateralBalanceX18);
    amountToTransfer = marginCollateralConfiguration.convertUd60x18ToTokenAmount(marginCollateralBalanceX18);
    ​
    IERC20(collateralType).safeTransfer(recipient, amountToTransfer);
    ​
    withdrawnMarginUsdX18 = marginCollateralPriceUsdX18.mul(marginCollateralBalanceX18);
    isMissingMargin = true;
    }

  1. amountToTransfer is 0 as he doesn't have any balance of that collateral

  2. The safeTransfer() will revert if a revert on zero amount transfer token is used or if any of the allowed tokens upgrade to a new implementation

While the likelihood of that is not high, the impact will be critical as liquidations will fail. We can see that the protocol tried to take care of the case where a user has no balance of a particular token:

if (ctx.marginCollateralBalanceX18.isZero()) continue;

However, they do not take care of it when the collateral has been used up after he has paid the liquidation fees. This issue also has an impact now and that is the fact that it does unnecessary computations and function calls when it will have no effect as he doesn't have any collateral (calling withdrawMarginUsd(), which computes if checks, does calculations, calls withdraw(), converts tokens, transfers tokens, writes to memory and storage multiple times, reads from storage multiple times). This will make the function much more gas intensive than required.

Impact

Liquidations could revert

Tools Used

Manual Review

Recommendations

Whenever you get in an if check and call withdrawMarginUsd(), check the isMissingMargin variable and continue if it's true, that means that the collateral was used up.

Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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