Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Liquidation function cannot seize the Required margin and PNL in some cases which will affect the marketmaking engine and it state will not be updated with the right margin.

bigsam

Summary

Because of an extreme case that pnl/profit can be positive and the account can be liquidatable in this state the contract will not receive the PNL and required margin of the user because the pnl has not yet been credited to the users margin balance hence, apart from the liquidation fee and some dust amounts the pnl and required margins are not recovered from the user.

Vulnerability Details

There is no check or settlement for pnl that are greater than 0.

e.g alice opens a long

  1. 1000 USD

  2. 1000 USD - pnl the profit has reached 100%

  3. Alice decide to reduce her position balance and not close the position

  4. Alice withdraws 950 USD from her balance

  5. now she has 1000 pnl + (50 *ltv)

  6. roughly let's say 1040 USD

  7. Alice leave this position and the margin falls has the price of the long dump against her position

  8. now she has 40 USD ==> buffered margin

  9. And 150 USD

  10. keeper calls to liquidate Alice because the PNL profit has not been added to her margin the liquidation process will only take away 50 USD from alice balance and the remaining 150 USD in profit will be lost and unaccounted for.

  11. this occurs because we only take margin from ALice balance not settling the positive PNL as done by settlement branch will make this error occur

Looking at the settlement branch

// if trader's old position had positive pnl then credit that to the trader
@audit>> if (ctx.pnlUsdX18.gt(SD59x18_ZERO)) {
IMarketMakingEngine marketMakingEngine = IMarketMakingEngine(perpsEngineConfiguration.marketMakingEngine);
​
ctx.marginToAddX18 =
marketMakingEngine.getAdjustedProfitForMarketId(marketId, ctx.pnlUsdX18.intoUD60x18().intoUint256());
​
tradingAccount.deposit(perpsEngineConfiguration.usdToken, ctx.marginToAddX18);
​
// mint settlement tokens credited to trader; tokens are minted to
// address(this) since they have been credited to the trader's margin
marketMakingEngine.withdrawUsdTokenFromMarket(marketId, ctx.marginToAddX18.intoUint256());
}
​
// add the market id to the market ids array
ctx.marketIds = new uint256[](1);
ctx.marketIds[0] = marketId;
​
// pay order/settlement fees and deduct collateral
// if trader's old position had negative pnl
tradingAccount.deductAccountMargin(
TradingAccount.DeductAccountMarginParams({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: perpsEngineConfiguration.marginCollateralRecipient,
orderFeeRecipient: perpsEngineConfiguration.orderFeeRecipient,
settlementFeeRecipient: perpsEngineConfiguration.settlementFeeRecipient
}),
pnlUsdX18: ctx.pnlUsdX18.lt(SD59x18_ZERO) ? ctx.pnlUsdX18.abs().intoUD60x18() : UD60x18_ZERO,
orderFeeUsdX18: ctx.orderFeeUsdX18,
settlementFeeUsdX18: ctx.settlementFeeUsdX18,
marketIds: ctx.marketIds,
accountPositionsNotionalValueX18: new UD60x18[](1) // when we have only one market id, this property
// isn't used, so we can pass an empty array
})

Now look at the liquidation branch we do not settle before calling deduct margin

// deduct maintenance margin from the account's collateral
// settlementFee = liquidationFee
@audit>> ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin(
TradingAccount.DeductAccountMarginParams({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: perpsEngineConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
@audit>> settlementFeeRecipient: perpsEngineConfiguration.liquidationFeeRecipient
}),
@audit>> pnlUsdX18: ctx.accountTotalUnrealizedPnlUsdX18.abs().intoUD60x18().add(
ctx.requiredMaintenanceMarginUsdX18
),
orderFeeUsdX18: UD60x18_ZERO,
@audit>> settlementFeeUsdX18: ctx.liquidationFeeUsdX18,
marketIds: ctx.activeMarketsIds,
accountPositionsNotionalValueX18: ctx.accountPositionsNotionalValueX18
})
);
​

call to deduct

function deductAccountMargin(
Data storage self,
DeductAccountMarginParams memory params
)
internal
returns (UD60x18 marginDeductedUsdX18)
{
// working data
DeductAccountMarginContext memory ctx;
​
// fetch storage slot for perps engine configuration
PerpsEngineConfiguration.Data storage perpsEngineConfiguration = PerpsEngineConfiguration.load();
​
// fetch storage slot for market making engine
IMarketMakingEngine marketMakingEngine = IMarketMakingEngine(perpsEngineConfiguration.marketMakingEngine);
​
// cache collateral liquidation priority length
uint256 cachedCollateralLiquidationPriorityLength =
perpsEngineConfiguration.collateralLiquidationPriority.length();
​
// loop through configured collateral types
for (uint256 i; i < cachedCollateralLiquidationPriorityLength; i++) {
// get ith collateral type
address collateralType = perpsEngineConfiguration.collateralLiquidationPriority.at(i);
​
// fetch storage slot for this collateral's config config
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
​
// get trader's collateral balance for this collateral, scaled to 18 decimals
ctx.marginCollateralBalanceX18 = getMarginCollateralBalance(self, collateralType);
​
// skip to next loop iteration if trader hasn't deposited any of this collateral
//we skip if balance is zero
@audit>>>> if (ctx.marginCollateralBalanceX18.isZero()) continue; // @audit Issue NOTE
​
// get this collateral's USD price
ctx.marginCollateralPriceUsdX18 = marginCollateralConfiguration.getPrice();
​
// if:
// settlement fee > 0 AND
// amount of settlement fee paid so far < settlement fee
if (
params.settlementFeeUsdX18.gt(UD60x18_ZERO)
&& ctx.settlementFeeDeductedUsdX18.lt(params.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,
params.settlementFeeUsdX18.sub(ctx.settlementFeeDeductedUsdX18),
params.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 (params.orderFeeUsdX18.gt(UD60x18_ZERO) && ctx.orderFeeDeductedUsdX18.lt(params.orderFeeUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin,) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
params.orderFeeUsdX18.sub(ctx.orderFeeDeductedUsdX18),
params.feeRecipients.orderFeeRecipient
);
ctx.orderFeeDeductedUsdX18 = ctx.orderFeeDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
// pnl logic same as settlement & order fee above
​
@audit>>> if (params.pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(params.pnlUsdX18)) {
@audit>>> (ctx.withdrawnMarginUsdX18, ctx.isMissingMargin, ctx.assetAmount) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
params.pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
address(this)
);
​
@audit>>> // verify if we have the asset to send to the market making engine
​
//0 funds are sent or lessthan we should by a lot of margin
@audit>>> if (ctx.assetAmount > 0) {
// create variable to cache the sum of all position usd
UD60x18 sumOfAllPositionsUsdX18;
​
// cache market ids length
uint256 cachedMarketIdsLength = params.marketIds.length;
​
// we need to sum the notional value of all active positions only if we're handling more than one
// market id
if (cachedMarketIdsLength > 1) {
for (uint256 j; j < params.accountPositionsNotionalValueX18.length; j++) {
sumOfAllPositionsUsdX18 =
sumOfAllPositionsUsdX18.add(params.accountPositionsNotionalValueX18[j]);
}
}
​
// loop into market ids
for (uint256 j; j < cachedMarketIdsLength; j++) {
// collateral native precision -> collateral zaros precision
UD60x18 collateralAmountX18 =
marginCollateralConfiguration.convertTokenAmountToUd60x18(ctx.assetAmount);
​
// if we have more than one market id, we need to calculate the percentage that will be
// deposited for this market
if (cachedMarketIdsLength > 1) {
// calculate the percentage to deposit to this market
UD60x18 percentDeductForThisMarketX18 =
params.accountPositionsNotionalValueX18[j].div(sumOfAllPositionsUsdX18);
collateralAmountX18 = collateralAmountX18.mul(percentDeductForThisMarketX18);
}
​
// deposit the collateral in the market making engine
@audit>>> marketMakingEngine.depositCreditForMarket(
uint128(params.marketIds[j]),
collateralType,
marginCollateralConfiguration.convertUd60x18ToTokenAmount(collateralAmountX18)
);
}
}
​
ctx.pnlDeductedUsdX18 = ctx.pnlDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
​
@audit>> // if there is no missing margin then exit the loop
@audit>> // since all amounts have been deducted
@audit>> if (!ctx.isMissingMargin) {
@audit>> break;
}
}
​
// output total margin deducted
@audit>> marginDeductedUsdX18 =
ctx.settlementFeeDeductedUsdX18.add(ctx.orderFeeDeductedUsdX18).add(ctx.pnlDeductedUsdX18);
}

Because we do not want to revert any liquidation we do not revert or call to ensure there is any strict check.

From the above code with a test it will be confirmed that the pnl is not handled at all also there will be no update in the marketmaking engine that we just incorporated.

Impact

The new update will not work in this scenario and the Pnl is lost, no one is claiming it. not the user and not the margin recipient and also no update in the Marketmaking engine will be possible.

Tools Used

Manual Reveiw

Recommendations

Settle all positive pnl as done in the settlement branch in the liquidation branch before calling the deduct margin

Updates

Lead Judging Commences

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Appeal created

bigsam Submitter
4 months ago
bigsam Submitter
4 months ago
inallhonesty Lead Judge
4 months ago
inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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