Summary
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/perpetuals/branches/LiquidationBranch.sol#L146-L161
When liquidateAccountsis invoked, it is checked whether the account to be liquidated is liquidatable
if (!TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, ctx.marginBalanceUsdX18)) {
continue;
}
In case it is, requiredMaintenanceMarginUsdX18 is passed as pnlUsdX18 to deductAccountMargin
Vulnerability Details
The whole required maintenance margin is deducted from the collateral deposited by the user being liquidated. This is unfair as the position might be just below the required margin. The difference between the required margin and the margin balance should be deducted instead.
PoC
In this PoC, a long position is created. Later, the price is reduced so the position is liquidatable. The required margin is just below the margin balance.
Add this test into liquidateAccounts.t.sol
function test_Deduct_Whole_Maintenance_Margin() external {
uint256 amountToDeposit = 100e18;
deal({ token: address(wstEth), to: users.naruto.account, give: amountToDeposit });
uint128 tradingAccountId = createAccountAndDeposit(amountToDeposit, address(wstEth));
uint128 marketId = 0;
int128 amount = 10e18;
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams(tradingAccountId, fuzzMarketConfig.marketId, amount)
);
bytes memory mockSignedReport =
getMockedSignedReport(fuzzMarketConfig.streamId, fuzzMarketConfig.mockUsdPrice);
address marketOrderKeeper = marketOrderKeepers[fuzzMarketConfig.marketId];
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
updateMockPriceFeed(uint128(fuzzMarketConfig.marketId), 86_488e18);
uint128[] memory accountsIds;
accountsIds = new uint128[](1);
accountsIds[0] = tradingAccountId;
changePrank({ msgSender: liquidationKeeper });
perpsEngine.liquidateAccounts({ accountsIds: accountsIds });
}
As can be seen in the event emitted, the requiredMaintenanceMarginUsd is just below the marginBalanceUsd but the whole requiredMaintenanceMarginUsd is deducted.
LogLiquidateAccount(keeper: ERC1967Proxy: [0x50795785161296B0A64914b4ee9285bAF70371Cd], tradingAccountId: 1, amountOfOpenPositions: 1,
requiredMaintenanceMarginUsd: 4324402162200000000000 [4.324e21], marginBalanceUsd: 4318532160000000000000 [4.318e21],
liquidatedCollateralUsd: 4329402162200000000000 [4.329e21], liquidationFeeUsd: 5000000000000000000 [5e18])
Tools Used
Foundry
Recommendations
ctx.liquidatedCollateralUsdX18 = tradingAccount.deductAccountMargin({
feeRecipients: FeeRecipients.Data({
marginCollateralRecipient: globalConfiguration.marginCollateralRecipient,
orderFeeRecipient: address(0),
settlementFeeRecipient: globalConfiguration.liquidationFeeRecipient
}),
- pnlUsdX18: requiredMaintenanceMarginUsdX18,
+ pnlUsdX18: requiredMaintenanceMarginUsdX18 - ctx.marginBalanceUsdX18,
orderFeeUsdX18: UD60x18_ZERO,
settlementFeeUsdX18: ctx.liquidationFeeUsdX18
});
