Summary
The perpetual Engine has a liquidation function in scope of this audit this was initially handled and no reverts will occur in the system but presently the new integration introduces some new risk that will cause liquidation to revert and the contract to remain insolvent.
The liquidation process is done in batches depending on the amount of liquidatable positions , DOS in one will cause a DOS in all, the new implementation introduces and enforces some checks that can revert the liquidation call.
Vulnerability Details
After every liquidation call the contract tries to settle the positions losses (PNL) and calls the trading library to properly settles this action this will also make calls to the Market Egine but this call is overall exposed and will cause liquidation to revert in some conditions this should never happen under any condition ideally.
Margin deductions
@audit>> 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
})
);
2.During settlement we call
if (params.pnlUsdX18.gt(UD60x18_ZERO) && ctx.pnlDeductedUsdX18.lt(params.pnlUsdX18)) {
(ctx.withdrawnMarginUsdX18, ctx.isMissingMargin, ctx.assetAmount) = withdrawMarginUsd(
self,
collateralType,
ctx.marginCollateralPriceUsdX18,
params.pnlUsdX18.sub(ctx.pnlDeductedUsdX18),
address(this)
);
if (ctx.assetAmount > 0) {
UD60x18 sumOfAllPositionsUsdX18;
uint256 cachedMarketIdsLength = params.marketIds.length;
if (cachedMarketIdsLength > 1) {
for (uint256 j; j < params.accountPositionsNotionalValueX18.length; j++) {
sumOfAllPositionsUsdX18 =
sumOfAllPositionsUsdX18.add(params.accountPositionsNotionalValueX18[j]);
}
}
for (uint256 j; j < cachedMarketIdsLength; j++) {
UD60x18 collateralAmountX18 =
marginCollateralConfiguration.convertTokenAmountToUd60x18(ctx.assetAmount);
if (cachedMarketIdsLength > 1) {
UD60x18 percentDeductForThisMarketX18 =
params.accountPositionsNotionalValueX18[j].div(sumOfAllPositionsUsdX18);
collateralAmountX18 = collateralAmountX18.mul(percentDeductForThisMarketX18);
}
@audit>> marketMakingEngine.depositCreditForMarket(
uint128(params.marketIds[j]),
collateralType,
marginCollateralConfiguration.convertUd60x18ToTokenAmount(collateralAmountX18)
);
}
}
ctx.pnlDeductedUsdX18 = ctx.pnlDeductedUsdX18.add(ctx.withdrawnMarginUsdX18);
}
3.The call then goes to the MarketMakingEngine
we revert when
when Marketid is pause this should not affect liquidation in any circumstance.
totaldelegationcreditusd==0
function depositCreditForMarket(
uint128 marketId,
address collateralAddr,
uint256 amount
)
external
onlyRegisteredEngine(marketId)
{
if (amount == 0) revert Errors.ZeroInput("amount");
Collateral.Data storage collateral = Collateral.load(collateralAddr);
@audit>> liquidation can revert>> collateral.verifyIsEnabled();
@audit>> load market not load live, see comment. liquidation WILL revert>> Market.Data storage market = Market.loadLive(marketId);
@audit>> liquidation can revert>> if (market.getTotalDelegatedCreditUsd().isZero()) {
revert Errors.NoDelegatedCredit(marketId);
}
UD60x18 amountX18 = collateral.convertTokenAmountToUd60x18(amount);
address usdToken = MarketMakingEngineConfiguration.load().usdTokenOfEngine[msg.sender];
address usdc = MarketMakingEngineConfiguration.load().usdc;
if (collateralAddr == usdToken) {
market.updateNetUsdTokenIssuance(unary(amountX18.intoSD59x18()));
} else {
if (collateralAddr == usdc) {
market.settleCreditDeposit(address(0), amountX18);
} else {
market.depositCredit(collateralAddr, amountX18);
}
}
IERC20(collateralAddr).safeTransferFrom(msg.sender, address(this), amount);
emit LogDepositCreditForMarket(msg.sender, marketId, collateralAddr, amount);
}
see
function unpauseMarket(uint128 marketId) external onlyOwner returns (bool success) {
success = LiveMarkets.load().addMarket(marketId);
if (success) emit LogMarketUnpaused(marketId);
}
function pauseMarket(uint128 marketId) external onlyOwner returns (bool success) {
success = LiveMarkets.load().removeMarket(marketId);
if (success) emit LogMarketPaused(marketId);
}
Impact
A paused market is one way of how liquidation can be DOSed among other reasons according to the initial implementation liquidation should never revert and based on the comment we should load the market and not check markets that are paused or unpaused. We should be able to liquidate a market paused/unpaused to prevent bad debt in the system. Other points of failure should also not occur to prevent bad debt formation.
Tools Used
Manual Review
Recommendations
for the paused market scenario, load the market and allow all market ids if they exist to be able to liquidate an account
review other new possible points of failure and ensure that liquidation takes place first and vault balancing should occur later.
