Submission Details
Severity:
The condition for `ctx.shouldUseMaintenanceMargin` in `OrderBranch::createMarketOrder()` is incomplete and does not work as expected. The check is performed with the wrong value.
Summary
The condition for ctx.shouldUseMaintenanceMargin in OrderBranch::createMarketOrder() is incomplete and does not work as expected. The check is performed with the wrong value.
Vulnerability Details
function createMarketOrder(CreateMarketOrderParams calldata params) external {
// working data
CreateMarketOrderContext memory ctx;
// revert for non-sensical zero size order
if (params.sizeDelta == 0) {
revert Errors.ZeroInput("sizeDelta");
}
// int128 -> SD59x18
ctx.sizeDeltaX18 = sd59x18(params.sizeDelta);
// fetch storage slot for global config
GlobalConfiguration.Data storage globalConfiguration = GlobalConfiguration.load();
// fetch storage slot for perp market's settlement config
SettlementConfiguration.Data storage settlementConfiguration =
SettlementConfiguration.load(params.marketId, SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID);
// determine whether position is being increased or not
ctx.isIncreasing = Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta);
// both markets and settlement can be disabled, however when this happens we want to:
// 1) allow open positions not subject to liquidation to decrease their size or close
// 2) prevent new positions from being opened & existing positions being increased
//
// the idea is to prevent a state where traders have open positions but are unable
// to reduce size or close even though they can still be liquidated; such a state
// would severly disadvantage traders
if (ctx.isIncreasing) {
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
// load existing trading account; reverts for non-existent account
// enforces `msg.sender == owner` so only account owner can place trades
TradingAccount.Data storage tradingAccount =
TradingAccount.loadExistingAccountAndVerifySender(params.tradingAccountId);
// find if account has active position in this market
@> ctx.isMarketWithActivePosition = tradingAccount.isMarketWithActivePosition(params.marketId);
// if the account doesn't have an active position in this market then
// this trade is opening a new active position in a new market, hence
// revert if this new position would put the account over the maximum
// number of open positions
if (!ctx.isMarketWithActivePosition) {
tradingAccount.validatePositionsLimit();
}
// fetch storage slot for perp market
PerpMarket.Data storage perpMarket = PerpMarket.load(params.marketId);
// enforce minimum trade size for this market
perpMarket.checkTradeSize(ctx.sizeDeltaX18);
// fetch storage slot for account's potential existing position in this market
Position.Data storage position = Position.load(params.tradingAccountId, params.marketId);
// int128 -> SD59x18
ctx.positionSizeX18 = sd59x18(position.size);
// enforce open interest and skew limits for target market
perpMarket.checkOpenInterestLimits(
ctx.sizeDeltaX18, ctx.positionSizeX18, ctx.positionSizeX18.add(ctx.sizeDeltaX18)
);
// fetch storage slot for trader's potential pending order
MarketOrder.Data storage marketOrder = MarketOrder.load(params.tradingAccountId);
(
ctx.marginBalanceUsdX18,
ctx.requiredInitialMarginUsdX18,
ctx.requiredMaintenanceMarginUsdX18,
ctx.orderFeeUsdX18,
ctx.settlementFeeUsdX18,
) = simulateTrade({
tradingAccountId: params.tradingAccountId,
marketId: params.marketId,
settlementConfigurationId: SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
sizeDelta: params.sizeDelta
});
// check maintenance margin if:
@> // 1) position is not increasing AND
@> // 2) existing position is being decreased in size
//
// when a position is under the higher initial margin requirement but over the
// lower maintenance margin requirement, we want to allow the trader to decrease
// their losing position size before they become subject to liquidation
//
// but if the trader is opening a new position or increasing the size
// of their existing position we want to ensure they satisfy the higher
// initial margin requirement
@> ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)
&& ctx.isMarketWithActivePosition;
@> ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? ctx.requiredMaintenanceMarginUsdX18 : ctx.requiredInitialMarginUsdX18;
// reverts if the trader can't satisfy the appropriate margin requirement
tradingAccount.validateMarginRequirement(
ctx.requiredMarginUsdX18, ctx.marginBalanceUsdX18, ctx.orderFeeUsdX18.add(ctx.settlementFeeUsdX18)
);
// reverts if a trader has a pending order and that pending order hasn't
// existed for the minimum order lifetime
marketOrder.checkPendingOrder();
// store pending order details
marketOrder.update({ marketId: params.marketId, sizeDelta: params.sizeDelta });
emit LogCreateMarketOrder(msg.sender, params.tradingAccountId, params.marketId, marketOrder);
}
Refer to the comments in the code to determine the conditions:
// check maintenance margin if:// 1) position is not increasing AND// 2) existing position is being decreased in size
The code logic is as follows:
ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)
&& ctx.isMarketWithActivePosition;
We can assume the following scenario
The original position is long:
oldPosition.size = 1e18.The size is increased:
params.sizeDelta = -3e18.
Then the new position is actuallynewPosition.size = 1e18 + -3e18 = -2e18. It can be seen that the actual position size increases after the position direction changes, but we getctx.shouldUseMaintenanceMargin = true. Andctx.requiredMaintenanceMarginUsdX18is used to initializectx.requiredMarginUsdX18for the following related checks. This is contrary to expectations
Poc
To view the value of the variable, we add the following test code to OrderBranch::createMarketOrder().
+ import {console} from "forge-std/console.sol";
ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? ctx.requiredMaintenanceMarginUsdX18 : ctx.requiredInitialMarginUsdX18;
+ console.log("ctx.requiredMarginUsdX18:",ctx.requiredMarginUsdX18.intoUint256());
+ console.log("ctx.requiredMaintenanceMarginUsdX18:",ctx.requiredMaintenanceMarginUsdX18.intoUint256());
+ console.log("ctx.requiredInitialMarginUsdX18:",ctx.requiredInitialMarginUsdX18.intoUint256());
Please add the test code to test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol and execute
function testOrderBranch_createMarketOrder_shouldUseMaintenanceMargin() 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 });
// Config first fill order
ctx.fuzzMarketConfig = getFuzzMarketConfig(ctx.marketId);
ctx.marketOrderKeeper = marketOrderKeepers[ctx.fuzzMarketConfig.marketId];
ctx.tradingAccountId = createAccountAndDeposit(ctx.marginValueUsd, address(usdz));
ctx.firstOrderSizeDelta = 1e18;
// The user creates an order for the first time
console.log("------------ The first time a user calls createMarketOrder() ------------");
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: ctx.firstOrderSizeDelta
})
);
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, ctx.fuzzMarketConfig.mockUsdPrice);
changePrank({ msgSender: ctx.marketOrderKeeper });
// keeper calls `fillMarketOrder()`. Prepare for the user to call `createMarketOrder()` for the second time
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
// User creates order for the second time
console.log("------------ The second time a user calls createMarketOrder() ------------");
changePrank({ msgSender: users.naruto.account });
ctx.secondOrderSizeDelta = -3e18;
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: ctx.secondOrderSizeDelta
})
);
}
// Ran 1 test for test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol:FillMarketOrder_Integration_Test
// [PASS] testOrderBranch_createMarketOrder_shouldUseMaintenanceMargin() (gas: 1212794)
// Logs:
// ------------ The first time a user calls createMarketOrder() ------------
//@> ctx.requiredMarginUsdX18: 1000000050000000000000
// ctx.requiredMaintenanceMarginUsdX18: 500000025000000000000
//@> ctx.requiredInitialMarginUsdX18: 1000000050000000000000
// changePrank is deprecated. Please use vm.startPrank instead.
// ------------ The second time a user calls createMarketOrder() ------------
// changePrank is deprecated. Please use vm.startPrank instead.
//@> ctx.requiredMarginUsdX18: 999999950000000000000
//@> ctx.requiredMaintenanceMarginUsdX18: 999999950000000000000
// ctx.requiredInitialMarginUsdX18: 1999999900000000000000
From the output, we can find that:
When the user creates an order for the first time,
ctx.requiredInitialMarginUsdX18is used to initializectx.requiredMarginUsdX18, which is correct.When the user creates an order for the second time, although the direction is to reduce the position, the actual position is increased compared to the previous state, but here
ctx.requiredMaintenanceMarginUsdX18is used incorrectly to initializectx.requiredMarginUsdX18.
Code Snippet
Impact
The condition for ctx.shouldUseMaintenanceMargin in OrderBranch::createMarketOrder() is incomplete and does not work as expected. The check is performed with the wrong value.
Tools Used
Manual Review
Recommendations
To sum up, we should consider adding a supplementary condition: ctx.sizeDeltaX18.abs() <= ctx.positionSizeX18.abs()
For example:
- ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)
- && ctx.isMarketWithActivePosition;
+ ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)
+ && ctx.isMarketWithActivePosition
+ && (ctx.sizeDeltaX18.abs() <= ctx.positionSizeX18.abs());
test again,it's work.
Ran 1 test for test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol:FillMarketOrder_Integration_Test
[PASS] testOrderBranch_createMarketOrder_shouldUseMaintenanceMargin() (gas: 1212794)
------------ The first time a user calls createMarketOrder() ------------
@> ctx.requiredMarginUsdX18: 1000000050000000000000
ctx.requiredMaintenanceMarginUsdX18: 500000025000000000000
@> ctx.requiredInitialMarginUsdX18: 1000000050000000000000
changePrank is deprecated. Please use vm.startPrank instead.
------------ The second time a user calls createMarketOrder() ------------
changePrank is deprecated. Please use vm.startPrank instead.
@> ctx.requiredMarginUsdX18: 1999999900000000000000
ctx.requiredMaintenanceMarginUsdX18: 999999950000000000000
@> ctx.requiredInitialMarginUsdX18: 1999999900000000000000
Updates
Lead Judging Commences
inallhonesty 10 months ago
Submission Judgement Published Assigned finding tags:
Disable market limitation can be bypassed
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.