Summary
When an account is liquidated, the protocol deducts the required maintenance margin instead of the account total unrealized Pnl.
Vulnerability Details
For example, suppose an account has collateral 1000$, and has already a position size 90 in ETh market. Very simply (ignoring the fill price and fees), the requiredMaintenanceMarginUsd would be 0.005 * 90 * 1000 = 450\$. If the price of ETH reduces by 8$, the uPnl would be 90 \* (-8) = -720$, and the account would be liquidatable because the marginBalanceUsd would be 1000 - 720 = 280$which is smaller thanrequiredMaintenanceMarginUsd. If this account is liquidated, the protocol deducts requiredMaintenanceMarginUsdfrom the collateral instead of the uPnl. In other words, the account is 720$ in loss, but the protocol deducts only 450$ and closes all its positions. By doing so, the total loss 720$ is not compensated, and the remaining loss720$ - 450$ = 270$\ will be remained for the protocol or liquidity providers.
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
pnlUsdX18: requiredMaintenanceMarginUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/LiquidationBranch.sol#L158
Please note the following scenario:
If ETH price decreases by 6$ from 1000$ to 994$, the owner of the account will not have incentive to close his position. Because, by closing the position the uPnl would be 90 * (-6) = -540$. But, if the owner of the account waits until the price decreases more to the point that the account becomes liquidatable, then when it is liquidated the deducted amount would be only 450$ (equal to requiredMaintenanceMarginUsd\) which is smaller than 540$. As a result, liquidation deducts less fund from collateral than the fund deducted when closing before being subject to liquidation.
In summary, the logic of deducting requiredMaintenanceMarginUsd from a liquidatable account instead of accountTotalUnrealizedPnlUsdX18 leads to different issues:
The loss would not be compensated by liquidating the account, and it will be a burden on the protocol or liquidity providers.
There will be no incentive for account holders to close their positions before being subject to liquidation.
PoC1
In the following PoC, after the ETH price is decreased by 5$, the accountTotalUnrealizedPnlUsdX18 is -450.002025, and requiredMaintenanceMarginUsdX18 is 447.30201285e18. So, the account is not liquidatable. When the position is closed before the ETH price decreases more, the getAccountMarginCollateralBalance returns 438.17748981e18. It means that, the total loss applied to the user is almost 1000 - 438 = 562. In the total loss, all the fee are also included.
function test_closingBeforeLiquidation()
external
givenTheSenderIsTheKeeper
givenTheMarketOrderExists
givenThePerpMarketIsEnabled
givenTheSettlementStrategyIsEnabled
givenTheReportVerificationPasses
whenTheMarketOrderIdMatches
givenTheDataStreamsReportIsValid
givenTheAccountWillMeetTheMarginRequirement
givenTheMarketsOILimitWontBeExceeded
{
TestFuzz_GivenThePnlIsPositive_Context memory ctx;
ctx.fuzzMarketConfig = getFuzzMarketConfig(ETH_USD_MARKET_ID);
uint256 marginValueUsd = 1000e18;
ctx.marketOrderKeeper = marketOrderKeepers[ctx.fuzzMarketConfig.marketId];
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
changePrank({ msgSender: users.naruto.account });
ctx.tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdz));
UD60x18 collat = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId, address(usdz));
console.log("balance before everything: ", unwrap(collat));
console.log("\nCreating first order\n");
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(90e18)
})
);
console.log("\nFilling first order\n");
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, ctx.fuzzMarketConfig.mockUsdPrice);
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
collat = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId, address(usdz));
console.log("balance after first order: ", unwrap(collat));
updateMockPriceFeed(ctx.fuzzMarketConfig.marketId, (1000 - 5) * 1e18);
uint128[] memory liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
console.log("account id: ", ctx.tradingAccountId);
console.log("liquidatableAccountsIds: ", liquidatableAccountsIds[0]);
changePrank({ msgSender: users.naruto.account });
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(-90e18)
})
);
console.log("\nFilling second order\n");
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, (1000 - 5) * 1e18);
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
collat = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId, address(usdz));
console.log("balance after liquidation: ", unwrap(collat));
}
PoC2
This time the user does not close its position, and waits until the price decreases more until it becomes liquidated.
In the following PoC, after the ETH price is decreased by 6$, the accountTotalUnrealizedPnlUsdX18 is -540.00243e18, and requiredMaintenanceMarginUsdX18 is 447.30201285e18. When it is liquidated, the getAccountMarginCollateralBalance returns 473.69766315e18. It means that only requiredMaintenanceMarginUsdX18 + liquidationFeeUsdX18 is deducted from the collateral, instead of accountTotalUnrealizedPnlUsdX18. So, the total loss applied to the user is almost 1000 - 473 = 527.
The amount of loss applied here is less than PoC1. So, the user who waits to be liquidated loses less than the user who closes his position before becoming subject to liquidation.
function test_liquidation()
external
givenTheSenderIsTheKeeper
givenTheMarketOrderExists
givenThePerpMarketIsEnabled
givenTheSettlementStrategyIsEnabled
givenTheReportVerificationPasses
whenTheMarketOrderIdMatches
givenTheDataStreamsReportIsValid
givenTheAccountWillMeetTheMarginRequirement
givenTheMarketsOILimitWontBeExceeded
{
TestFuzz_GivenThePnlIsPositive_Context memory ctx;
ctx.fuzzMarketConfig = getFuzzMarketConfig(ETH_USD_MARKET_ID);
uint256 marginValueUsd = 1000e18;
ctx.marketOrderKeeper = marketOrderKeepers[ctx.fuzzMarketConfig.marketId];
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
changePrank({ msgSender: users.naruto.account });
ctx.tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdz));
UD60x18 collat = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId, address(usdz));
console.log("balance before everything: ", unwrap(collat));
console.log("\nCreating first order\n");
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(90e18)
})
);
console.log("\nFilling first order\n");
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, ctx.fuzzMarketConfig.mockUsdPrice);
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
collat = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId, address(usdz));
console.log("balance after first order: ", unwrap(collat));
updateMockPriceFeed(ctx.fuzzMarketConfig.marketId, (1000 - 6) * 1e18);
uint128[] memory liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
console.log("account id: ", ctx.tradingAccountId);
console.log("liquidatableAccountsIds: ", liquidatableAccountsIds[0]);
console.log("\nLiquidating\n");
changePrank({ msgSender: liquidationKeeper });
uint128[] memory accountsIds = new uint128[](1);
accountsIds[0] = ctx.tradingAccountId;
perpsEngine.liquidateAccounts(accountsIds);
collat = perpsEngine.getAccountMarginCollateralBalance(ctx.tradingAccountId, address(usdz));
console.log("balance after liquidation: ", unwrap(collat));
}
Impact
Tools Used
Recommendations
Following modification is recommended:
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
pnlUsdX18: accountTotalUnrealizedPnlUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/LiquidationBranch.sol#L158
