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

Unfair Liquidation mechanism doesn't charge for negative PnL, causing the MarginCollateralRecipient to receive less than what it should.

0xstalin
## Summary
When liquidating an account, the account's PnL is only considered to compute the total marginBalanceUSD of the account, which is used to determine if the account is liquidable or not.
- If the account is liquidable, the PnL is not considered when deducting the margin from the account. This means, any negative PnL won't be deducted from the account, it will be simply ignored and not paid to the MarginCollateralRecipient.
​
​
## Vulnerability Details
When liquidating an account, the account's PnL is only considered to compute the total marginBalanceUSD of the account, which is used to determine if the account is liquidable or not.
- If the account is liquidable, the PnL is not considered when deducting the margin from the account. This means, any negative PnL won't be deducted from the account, it will be simply ignored and not paid to the MarginCollateralRecipient.
​
Let's make an example of 3 accounts that are liquidable, each of which has a different PnL as well as a different total adjustedBalanceUSD for the summation of the loanToValue of their collateral. Each of these accounts has the same requiredMargin and the same marginBalance after accounting for the PnL and totalAdjustedBalanceUSD.
​
1. Account 1 - 0 PnL
- This account will get deducted 1050 from its collateral which has a market value of 1200. All good, the account pays the correct amount for the liquidation.
```
requiredMarging: 1050
PnL: 0
​
marginBalance: 1000 - 0 ===> 1000
​
loanToValue: 1000
RealMarketValue: 1200
```
​
2. Account 2 - Positive PnL (100)
- This account will also get deducted 1050 from its collateral which has a market value of 1200. This account had a lower loanToValue (adjustedBalance) because it was a collateral with lower LTV than account 1, but, in the end, the account 1 and account 2 are correctly paying to the MarginCollateralRecipient
```
requiredMarging: 1050
PnL: 100
​
marginBalance: 900 + 100 ===> 1000
​
loanToValue: 900
RealMarketValue: 1200
​
Account gets deducted 1050
```
​
3. Account 3 - Negative PnL (-100)
- This account has negative PnL and collateral with better LTV, but, unfortunately, the account is liquidable because the marginBalance is not enough to cover the requiredMargin.
- **The problem is, that this account will pay the same as Account1 and Account2, even though those accounts have a neutral and positive PnL respectively.** ***This account has a negative PnL, which, in perpetuals, the loss of one party is the gain of the other party.***
- In this case, those 100 negative PnL is the gain of the other side of the trade (including the fundingFee), but, because the liquidation is not charging the negative PnL, the other party won't receive those gains.
```
requiredMarging: 1050
PnL: -100
​
marginBalance: 1100 - 100 ===> 1000
​
loanToValue: 1100
RealMarketValue: 1200
```
​
As we can see on the previous examples, Account3 should have paid 1150 to cover the requiredMargin and the negative PnL that was generated while the position was opened, otherwise, the fundingFees and the gains of the other party of the trade will be lost.
​
On the below code snippet we can see that [the PnL is not accounted for when deducting the margin from the liquidated account's balance.](https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/LiquidationBranch.sol#L146-L161)
​
```
/// @param accountsIds The list of accounts to liquidate
function liquidateAccounts(uint128[] calldata accountsIds) external {
...
​
for (uint256 i; i < accountsIds.length; i++) {
...
​
// get account's required maintenance margin & unrealized PNL
(, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
//@audit => marginBalanceUsd is the sumation of the collateral's loanToValue in USD +/- the uPnL
// 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;
}
​
// 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
}),
//@audit-issue => Not charging the negative PnL to the amount of collateral that will be deducted from the liquidated account!
pnlUsdX18: requiredMaintenanceMarginUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
​
...
}
}
```
​
## Impact
The profit of the other side of the trade will be lost when accounts are liquidated because the negative PnL of the TradingAccounts is not deducted from their collateral balance.
​
## Tools Used
Manual Audit.
​
## Recommendations
Do not ignore the negative PnL when deducting the margin from the liquidated account, make sure to charge and deduct it from the liquidated account's collateral.
​
```
/// @param accountsIds The list of accounts to liquidate
function liquidateAccounts(uint128[] calldata accountsIds) external {
...
​
for (uint256 i; i < accountsIds.length; i++) {
...
​
// get account's required maintenance margin & unrealized PNL
(, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
​
...
​
if (!TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, ctx.marginBalanceUsdX18)) {
continue;
}
​
// deduct maintenance margin from the account's collateral
// settlementFee = liquidationFee
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
...
}),
//@audit => If negative PnL, make sure to also deduct it from the liquidated account's collateral, if PnL is >= 0, only deduct the requiredMaintenanceMargin.
+ pnlUsdX18: accountTotalUnrealizedPnlUsdX18.lt(SD59x18_ZERO) ? requiredMaintenanceMarginUsdX18.add(accountTotalUnrealizedPnlUsdX18.abs().intoUD60x18()) : requiredMaintenanceMarginUsdX18,
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.

Give us feedback!