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

Incorrect `pnlUsdX18` in liquidation process leads to incorrect `liquidatedCollateralUsdX18` amount

nisedo

Vulnerability Details

In the LiquidationBranch.sol contract, the liquidateAccounts() function is responsible for liquidating accounts that have fallen below the required maintenance margin. This function iterates through a list of account IDs, checks if each account is liquidatable, and if so, proceeds with the liquidation process.

A critical part of this process is the call to deductAccountMargin(), which is responsible for deducting the required margin from the account's collateral. However, there is a discrepancy in the parameters passed to this function.

The pnlUsdX18 parameter in the deductAccountMargin() call is currently set to requiredMaintenanceMarginUsdX18:

LiquidationBranch.sol#L150-L161

// deduct maintenance margin from the account's collateral
// settlementFee = liquidationFee
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
>> pnlUsdX18: requiredMaintenanceMarginUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});

However, this value represents the required maintenance margin, not the account's unrealized PnL. The correct value to use here should be accountTotalUnrealizedPnlUsdX18, which is calculated earlier in the function.

TradingAccount.sol#L481-L494

/// @notice Deducts the account's margin to pay for the settlement fee, order fee, and realize the pnl.
/// @param self The trading account storage pointer.
/// @param feeRecipients The fee recipients.
>> /// @param pnlUsdX18 The total unrealized PnL of the account.
/// @param settlementFeeUsdX18 The total settlement fee to be deducted from the account.
/// @param orderFeeUsdX18 The total order fee to be deducted from the account.
/// @return marginDeductedUsdX18 The total margin deducted from the account.
function deductAccountMargin(
Data storage self,
FeeRecipients.Data memory feeRecipients,
>> UD60x18 pnlUsdX18,
UD60x18 settlementFeeUsdX18,
UD60x18 orderFeeUsdX18
)

This incorrect parameter leads to a scenario where accounts are over-liquidated if their required maintenance margin is greater than their unrealized PnL. In such cases, the protocol would be deducting more collateral from the account than necessary, causing undue losses to the user being liquidated. Otherwise, it will lead to user being under-liquidated if their required maintenance margin is lower than their unrealized PnL, leading to a loss for the procotol.

Impact

Accounts may be over-liquidated, causing losses to users, or under-liquidated, causing losses to the protocol.

Proof of Concept

Over-liquidation:

  1. An account's position becomes liquidatable.

  2. The liquidateAccounts() function is called with this account's ID.

  3. The function calculates requiredMaintenanceMarginUsdX18 and accountTotalUnrealizedPnlUsdX18.

  4. When calling deductAccountMargin(), requiredMaintenanceMarginUsdX18 is passed instead of accountTotalUnrealizedPnlUsdX18.

  5. If requiredMaintenanceMarginUsdX18 > accountTotalUnrealizedPnlUsdX18, more collateral than necessary is deducted.

  6. The user suffers unnecessary losses due to over-liquidation.

Under-liquidation:

  1. An account's position becomes liquidatable.

  2. The liquidateAccounts() function is called with this account's ID.

  3. The function calculates requiredMaintenanceMarginUsdX18 and accountTotalUnrealizedPnlUsdX18.

  4. When calling deductAccountMargin(), requiredMaintenanceMarginUsdX18 is passed instead of accountTotalUnrealizedPnlUsdX18.

  5. If requiredMaintenanceMarginUsdX18 < accountTotalUnrealizedPnlUsdX18, fewer collateral than necessary is deducted.

  6. The protocol suffers losses due to under-liquidation of the user.

Recommendations

Replace the pnlUsdX18 parameter in the deductAccountMargin() function call with accountTotalUnrealizedPnlUsdX18:

// deduct maintenance margin from the account's collateral
// settlementFee = liquidationFee
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
- pnlUsdX18: requiredMaintenanceMarginUsdX18,
+ pnlUsdX18: accountTotalUnrealizedPnlUsdX18.abs().intoUD60x18(),
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
Updates

Lead Judging Commences

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

deductAccountMargin() treats `required maintenance margin` as the `unrealized PnL` because it uses `requiredMaintenanceMarginUsdX18` in place of `pnlUsdX18`

Support

FAQs

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