Submission Details
Severity:
Wrong Pnlusdx18 amount calculated to be removed by the liquidation function
Summary
Whenever a position is liqudatable that means the margin balance as fallen below the mmr. A position is max profit can reduce their collateral balance up to the point when only liquidation fee is allowed to remain in the users balance but the liquidation profit doesn't correct handle this scenario where PNL is positive, hence instead of increase the users balance by the pnl we decrease the user's collateral balance.
Vulnerability Details
PNL handling in the liquidation function
// deduct maintenance margin from the account's collateral
// settlementFee = liquidationFee
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin(
TradingAccount.DeductAccountMarginParams({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: perpsEngineConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: perpsEngineConfiguration.liquidationFeeRecipient
}),
@audit>> pnlUsdX18: ctx.accountTotalUnrealizedPnlUsdX18.abs().intoUD60x18().add(
ctx.requiredMaintenanceMarginUsdX18
),
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18,
marketIds: ctx.activeMarketsIds,
accountPositionsNotionalValueX18: ctx.accountPositionsNotionalValueX18
})
);
instead of handling this as it was done during settlement
settlement handling below
// 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
}),
@audit>> see settlement>> 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
})
);
We should only return an abs value of the profit when the pnl is less than 0 and not always
/// @notice Withdraws available margin collateral from the given trading account.
/// @param tradingAccountId The trading account id.
/// @param collateralType The margin collateral address.
/// @param amount The UD60x18 amount of margin collateral to withdraw.
function withdrawMargin(uint128 tradingAccountId, address collateralType, uint256 amount) external {
// fetch storage slot for this collateral's config config
MarginCollateralConfiguration.Data storage marginCollateralConfiguration =
MarginCollateralConfiguration.load(collateralType);
// load existing trading account; reverts for non-existent account
// enforces `msg.sender == owner` so only account owner can withdraw
TradingAccount.Data storage tradingAccount =
TradingAccount.loadExistingAccountAndVerifySender(tradingAccountId);
// reverts if a trader has a pending order
_requireNoActiveMarketOrder(tradingAccountId);
// convert uint256 -> UD60x18; scales input amount to 18 decimals
UD60x18 amountX18 = marginCollateralConfiguration.convertTokenAmountToUd60x18(amount);
// enforce converted amount > 0
_requireAmountNotZero(amountX18);
// enforces that user has deposited enough collateral of this type to withdraw
_requireEnoughMarginCollateral(tradingAccount, collateralType, amountX18);
// deduct amount from trader's collateral balance
tradingAccount.withdraw(collateralType, amountX18);
// load account required initial margin requirement & unrealized USD profit/loss
// ignores "required maintenance margin" output parameter
(UD60x18 requiredInitialMarginUsdX18,, SD59x18 accountTotalUnrealizedPnlUsdX18) =
tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
// get trader's margin balance
SD59x18 marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
// check against initial margin requirement as initial margin > maintenance margin
// hence prevent the user from withdrawing all the way to the maintenance margin
// so that they couldn't be liquidated very soon afterwards if their position
// goes against them even a little bit
tradingAccount.validateMarginRequirement(requiredInitialMarginUsdX18, marginBalanceUsdX18, UD60x18_ZERO);
// cache whether the trader has an active position or not
bool userHasOpenPosition = !requiredInitialMarginUsdX18.isZero();
@audit>> if (userHasOpenPosition) {
// load perps engine configuration from storage
PerpsEngineConfiguration.Data storage perpsEngineConfiguration = PerpsEngineConfiguration.load();
// save the trader's magin balance without taking the unrealized pnl into account
@audit>> SD59x18 marginBalanceUsdWithoutUnrealizedPnlUsdX18 = tradingAccount.getMarginBalanceUsd(SD59x18_ZERO);
// verify if after the withdrawal the account will still own enough collateral to cover the liquidation
// fee value
@audit>> if (
marginBalanceUsdWithoutUnrealizedPnlUsdX18.lt(
sd59x18(int128(perpsEngineConfiguration.liquidationFeeUsdX18))
)
) {
revert Errors.NotEnoughCollateralForLiquidationFee(perpsEngineConfiguration.liquidationFeeUsdX18);
}
}
// finally send the tokens
IERC20(collateralType).safeTransfer(msg.sender, amount);
emit LogWithdrawMargin(msg.sender, tradingAccountId, collateralType, amount);
}
Impact
Wrong accounting will cause PNL removal from the balance even though we are in profit. Pnl would be part of MMR already if positive.
Tools Used
manual review
Recommendations
only add pnl as done in the settlement branch when the pnl is less than 0.
Updates
Prize pool breakdown
Total prize
70,000 USDC
nSLOC
3,25821 USDC / LOC
66,500
3,500
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.