Summary
Market orders lack price checks. If prices fluctuate drastically, it will cause unnecessary losses to users.
Vulnerability Details
Users create orders through OrderBranch::createMarketOrder(), but the order data will only save marketId, sizeDelta, timestamp.
marketOrder.update({ marketId: params.marketId, sizeDelta: params.sizeDelta });
function update(Data storage self, uint128 marketId, int128 sizeDelta) internal {
self.marketId = marketId;
self.sizeDelta = sizeDelta;
self.timestamp = uint128(block.timestamp);
}
struct Data {
uint128 marketId;
int128 sizeDelta;
uint128 timestamp;
}
When keeper calls SettlementBranch::fillMarketOrder(), it directly gets the current fill price and calls SettlementBranch::_fillOrder() without any checks.
function fillMarketOrder(
uint128 tradingAccountId,
uint128 marketId,
bytes calldata priceData
)
external
onlyMarketOrderKeeper(marketId)
{
FillMarketOrder_Context memory ctx;
SettlementConfiguration.Data storage settlementConfiguration =
SettlementConfiguration.load(marketId, SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID);
MarketOrder.Data storage marketOrder = MarketOrder.loadExisting(tradingAccountId);
if (marketId != marketOrder.marketId) {
revert Errors.OrderMarketIdMismatch(marketId, marketOrder.marketId);
}
PerpMarket.Data storage perpMarket = PerpMarket.load(marketId);
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
(ctx.bidX18, ctx.askX18) =
settlementConfiguration.verifyOffchainPrice(priceData, globalConfiguration.maxVerificationDelay);
ctx.sizeDeltaX18 = sd59x18(marketOrder.sizeDelta);
ctx.isBuyOrder = ctx.sizeDeltaX18.gt(SD59x18_ZERO);
ctx.indexPriceX18 = ctx.isBuyOrder ? ctx.askX18 : ctx.bidX18;
@> ctx.fillPriceX18 = perpMarket.getMarkPrice(ctx.sizeDeltaX18, ctx.indexPriceX18);
_fillOrder(
tradingAccountId,
marketId,
SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
ctx.sizeDeltaX18,
@> ctx.fillPriceX18
);
marketOrder.clear();
}
We can imagine a scenario:
The user creates a market order with a target price of 1e18.
When the keeper executes the order, the price fluctuates greatly, even far exceeding the user's target price. However, the user's margin balance is sufficient to support the transaction, and the transaction will still be completed, but the price is not the price the user expected. This transaction is bound to cause unnecessary losses to the user.
Poc
Please add the test code to test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol and execute
function test_createMarketOrder_and_fillMarketOrder() public {
Test_GivenTheMarginBalanceUsdIsOverTheMaintenanceMarginUsdRequired_Context memory ctx;
ctx.marketId = BTC_USD_MARKET_ID;
ctx.marginValueUsd = 100_000e18;
deal({ token: address(usdz), to: users.naruto.account, give: ctx.marginValueUsd });
ctx.fuzzMarketConfig = getFuzzMarketConfig(ctx.marketId);
ctx.marketOrderKeeper = marketOrderKeepers[ctx.fuzzMarketConfig.marketId];
ctx.tradingAccountId = createAccountAndDeposit(ctx.marginValueUsd, address(usdz));
ctx.firstOrderSizeDelta = 10e18;
assertEq(ctx.fuzzMarketConfig.mockUsdPrice,100000000000000000000000);
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: ctx.firstOrderSizeDelta
})
);
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, ctx.fuzzMarketConfig.mockUsdPrice * 12000 / 10000 );
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
Position.State memory positionState = perpsEngine.getPositionState(ctx.tradingAccountId,ctx.fuzzMarketConfig.marketId,ctx.fuzzMarketConfig.mockUsdPrice);
assertGt(positionState.entryPriceX18.intoUint256(),120000000000000000000000);
}
Code Snippet
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L351-L352
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/MarketOrder.sol#L43-L47
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/MarketOrder.sol#L19-L23
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L103-L166
Impact
Market orders lack price checks. If prices fluctuate drastically, it will cause unnecessary losses to users.
Tools Used
Manual Review
Recommendations
Add a target price item similar to SettlementBranch::fillOffchainOrders()::ctx.offchainOrder.targetPrice in the order data
// MarketOrder::
struct Data {
uint128 marketId;
int128 sizeDelta;
uint128 timestamp;
+ uint128 targetPrice;
}
-
Add targetPrice parameter update logic in OrderBranch::createMarketOrder() and MarketOrder::update()
-
Add the check logic similar to that in SettlementBranch::fillOffchainOrders() in SettlementBranch::fillMarketOrder()
ctx.fillPriceX18 = perpMarket.getMarkPrice(ctx.sizeDeltaX18, ctx.indexPriceX18);
// if the order increases the trading account's position (buy order), the fill price must be less than or
// equal to the target price, if it decreases the trading account's position (sell order), the fill price
// must be greater than or equal to the target price.
+ ctx.isFillPriceValid = (ctx.isBuyOrder && marketOrder.targetPrice >= ctx.fillPriceX18.intoUint256())
+ || (!ctx.isBuyOrder && marketOrder.targetPrice <= ctx.fillPriceX18.intoUint256());
+ if (!ctx.isFillPriceValid) {
+ revert("FillPrice error");
+ }
// perform the fill
_fillOrder(
tradingAccountId,
marketId,
SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
ctx.sizeDeltaX18,
ctx.fillPriceX18
);
// reset pending order details
marketOrder.clear();
