Summary
The settlement process lacks a critical check to ensure that the position being filled is not liquidatable. Although the "isLiquidatable" check may pass during order creation, sudden price movements and the asynchronous nature of order fulfillment via keepers can lead to a scenario where the order becomes liquidatable by the time it is filled. This means the filled position can immediately become liquidatable, causing an instant loss to the user.
Vulnerability Details
During order creation, the order is simulated using the simulateTrade function, which ensures the simulated trade is not liquidatable, as shown below:
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/perpetuals/branches/OrderBranch.sol#L133-L134
if (TradingAccount.isLiquidatable(ctx.previousRequiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
revert Errors.AccountIsLiquidatable(tradingAccountId);
}
This check ensures that once the trade is filled upon settlement, it is not immediately liquidatable, preventing instant loss to the user.
However, this validation is missing during the settlement process. Due to the asynchronous nature of settlement triggered by keepers, the settlement price might deviate, especially during volatile market conditions. This increases the likelihood that an order, that was not liquidatable at creation, becomes liquidatable at settlement.
The market order keeper function that fills a market order is shown below:
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/external/chainlink/keepers/market-order/MarketOrderKeeper.sol#L169
function performUpkeep(bytes calldata performData) external onlyForwarder {
(bytes memory signedReport, bytes memory extraData) = abi.decode(performData, (bytes, bytes));
uint128 tradingAccountId = abi.decode(extraData, (uint128));
MarketOrderKeeperStorage storage self = _getMarketOrderKeeperStorage();
(IPerpsEngine perpsEngine, uint128 marketId) = (self.perpsEngine, self.marketId);
@> perpsEngine.fillMarketOrder(tradingAccountId, marketId, signedReport);
}
There are no checks here to verify whether the position being filled is liquidatable. The fillMarketOrder function, triggered by this process, also does not check whether the filled position will result in the user being liquidatable.
Proof Of Concept
Add the following code to liquidateAccounts.t.sol
import { OrderBranch } from "@zaros/perpetuals/branches/OrderBranch.sol";
function testGivenMarketOrderIsCreated_whenFillingOrder_shouldNotAcceptOrderIfItsLiquitadable() public {
uint256 marginValueUsdc = 100_000e6;
int128 userPositionSizeDelta = 10e18;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
bytes memory mockSignedReport = _creteMarketOrder(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, userPositionSizeDelta
);
updateMockPriceFeed(BTC_USD_MARKET_ID, MOCK_BTC_USD_PRICE/2);
address marketOrderKeeper = marketOrderKeepers[BTC_USD_MARKET_ID];
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tradingAccountId, BTC_USD_MARKET_ID, mockSignedReport);
vm.expectEmit({
checkTopic1: true,
checkTopic2: true,
checkTopic3: false,
checkData: false,
emitter: address(perpsEngine)
});
emit LiquidationBranch.LogLiquidateAccount({
keeper: liquidationKeeper,
tradingAccountId: tradingAccountId,
amountOfOpenPositions: 0,
requiredMaintenanceMarginUsd: 0,
marginBalanceUsd: 0,
liquidatedCollateralUsd: 0,
liquidationFeeUsd: 0
});
_liquidateOneAccount(tradingAccountId);
}
function _creteMarketOrder(
uint128 marketId,
bytes32 streamId,
uint256 mockUsdPrice,
uint128 tradingAccountId,
int128 sizeDelta
) internal returns (bytes memory mockSignedReport) {
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: marketId,
sizeDelta: sizeDelta
})
);
mockSignedReport = getMockedSignedReport(streamId, mockUsdPrice);
}
function _liquidateOneAccount(uint128 tradingAccountId) internal {
changePrank({ msgSender: liquidationKeeper });
skip(1 hours);
uint128[] memory liquidableAccountIds = new uint128[](1);
liquidableAccountIds[0] = tradingAccountId;
perpsEngine.liquidateAccounts(liquidableAccountIds);
}
run: forge test --match-test testGivenMarketOrderIsCreated_whenFillingOrder_shouldNotAcceptOrderIfItsLiquitadable
Output:
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.85ms (1.01ms CPU time)
The test shows the following:
Bob creates an account, deposits collateral, and places a market order.
The collateral price suddenly drops, making Bob's position liquidatable.
The keeper calls fillMarketOrder, and the order is successfully filled.
A bot immediately liquidates the position.
The call doesn't revert.
Impact
Loss of Funds: Not verifying if a position is liquidatable before filling it will cause the user to incur an instant loss.
Tools Used
Manual Review & Foundry
Recommendations
Immediately after the order is filled at the end of the _fillOrder function and before the emit statement, you can perform the isLiquidatable check as shown below. This will cause the fill order to fail if the position is liquidatable.
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/perpetuals/branches/SettlementBranch.sol#L505
+ {
+ // // get account's required maintenance margin & unrealized PNL
+ (, UD60x18 requiredMaintenanceMarginUsdX18, SD59x18 accountTotalUnrealizedPnlUsdX18) =
+ tradingAccount.getAccountMarginRequirementUsdAndUnrealizedPnlUsd(0, SD59x18_ZERO);
+
+ // use unrealized PNL to calculate & output account's margin balance
+ SD59x18 marginBalanceUsdX18 = tradingAccount.getMarginBalanceUsd(accountTotalUnrealizedPnlUsdX18);
+
+ // prevent liquidatable accounts from trading
+ if (TradingAccount.isLiquidatable(requiredMaintenanceMarginUsdX18, marginBalanceUsdX18)) {
+ revert Errors.AccountIsLiquidatable(tradingAccountId);
+ }
+ }
+
emit LogFillOrder(
msg.sender,
tradingAccountId,
Run the PoC again: Result ✅
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 16.90ms (2.29ms CPU time)
[FAIL. Reason: AccountIsLiquidatable(1)]
