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

PnL calculation for the user's old position is wrong in `OrderBranch::createMarketOrder`

aamirusmani1552

Summary

In OrderBranch::createMarketOrder, there is a check to see if the previously user was liquiduidatable or not. And to do this users margin is calculated. But to this margin, user's PnL is added which includes the new trade impact.

Vulnerability Details

This is the calcuation which is done to calculate the user's current 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)) {
console2.log("we are here");
revert Errors.AccountIsLiquidatable(tradingAccountId);
}
}

GitHub: https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L123C1-L138C1

In the first line, we are making call to getAccountMarginRequirementUsdAndUnrealizedPnlUsd(marketId, ctx.sizeDeltaX18)(...) by passing also the sizeDeltaX18. Because of this, the PnL will take into account the new change in position as well. And the new mark price will also be calculated by taking into account this sizeDeltaX18.

Then this calculated PnL will be added to the users current margin and the call will be made to the isLiquidatable(...) to check if the previous position of the user was liquidatable. But since this new calculated PnL includes the new position delta, it will be wrong. And there are chances that it might generate loss and might show user's position liquidatable or vice versa.

Impact

The calculated PnL for old position will be wrong.

Tools Used

  • Manual Review

Recommendations

Calculate the user's PnL based on the current positions only. Do not include the trade impact in it.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Appeal created

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

Including the positive uPnL of new position in the margin balance leads to an artificial increase of the total margin balance, which leads to the ability to drain the protocol

Support

FAQs

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