Submission Details
Severity:
Arithmetic over/underflow
Summary: The checkOpenInterestLimits function in PerpMarket.sol is encountering an arithmetic issue. Specifically, the newOpenInterest calculation cannot handle negative values, which leads to arithmetic overflows or underflows. The problematic calculation is: currentOpenInterest.sub(oldPositionSize.abs().intoUD60x18()).add(newPositionSize.abs().intoUD60x18())
The issue arises because the sub function used in Helpers.sol can result in a negative value, which is not handled properly.
/// @notice Implements the checked subtraction operation (-) in the UD60x18 type.
function sub(UD60x18 x, UD60x18 y) pure returns (UD60x18 result) {
result = wrap(x.unwrap() - y.unwrap());
}
function checkOpenInterestLimits(
Data storage self,
SD59x18 sizeDelta,
SD59x18 oldPositionSize,
SD59x18 newPositionSize
)
internal
view
returns (UD60x18 newOpenInterest, SD59x18 newSkew)
{
// load max & current open interest for this perp market uint128 -> UD60x18
UD60x18 maxOpenInterest = ud60x18(self.configuration.maxOpenInterest);
UD60x18 currentOpenInterest = ud60x18(self.openInterest);
// calculate new open interest which would result from proposed trade
// by subtracting old position size then adding new position size to
// current open interest
newOpenInterest =
currentOpenInterest.sub(oldPositionSize.abs().intoUD60x18()).add(newPositionSize.abs().intoUD60x18());
// if new open interest would be greater than this market's max open interest,
// we still want to allow trades as long as they decrease the open interest. This
// allows traders to reduce/close their positions in markets where protocol admins
// have reduced the max open interest to reduce the protocol's exposure to a given
// perp market
if (newOpenInterest.gt(maxOpenInterest)) {
// is the proposed trade reducing open interest?
bool isReducingOpenInterest = currentOpenInterest.gt(newOpenInterest);
// revert if the proposed trade isn't reducing open interest
if (!isReducingOpenInterest) {
revert Errors.ExceedsOpenInterestLimit(
self.id, maxOpenInterest.intoUint256(), newOpenInterest.intoUint256()
);
}
}
// load max & current skew for this perp market uint128 -> UD60x18 -> SD59x18
SD59x18 maxSkew = ud60x18(self.configuration.maxSkew).intoSD59x18();
// int128 -> SD59x18
SD59x18 currentSkew = sd59x18(self.skew);
// calculate new skew
newSkew = currentSkew.add(sizeDelta);
// similar logic to the open interest check; if the new skew is greater than
// the max, we still want to allow trades as long as they decrease the skew
if (newSkew.abs().gt(maxSkew)) {
bool isReducingSkew = currentSkew.abs().gt(newSkew.abs());
if (!isReducingSkew) {
revert Errors.ExceedsSkewLimit(self.id, maxSkew.intoUint256(), newSkew.intoInt256());
}
}
}
Vulnerability Details:
contract PerpMarket_CheckOpenInterestLimits_Unit_Test is Base_Test {
function setUp() public virtual override {
Base_Test.setUp();
changePrank({ msgSender: users.owner.account });
configureSystemParameters();
createPerpMarkets();
changePrank({ msgSender: users.naruto.account });
}
modifier whenTheNewOpenInterestIsGreaterThanTheMaxOpenInterest() {
_;
}
function testFuzz_CheckOpenInterestLimits_NegativeNewOpenInterest(
uint256 marketId,
uint256 currentOpenInterest,
uint256 oldPositionSize,
uint256 newPositionSize
) public {
// Bound the input values
marketId = bound(marketId, 0, type(uint8).max);
currentOpenInterest = bound(currentOpenInterest, 1, type(uint128).max); // Ensure non-zero
oldPositionSize = bound(oldPositionSize, currentOpenInterest + 1, type(uint128).max);
newPositionSize = bound(newPositionSize, 0, oldPositionSize - currentOpenInterest - 1);
// Skip invalid combinations
vm.assume(oldPositionSize > currentOpenInterest);
vm.assume(oldPositionSize > newPositionSize);
MarketConfig memory marketConfig = getFuzzMarketConfig(marketId);
// Convert to the required types
UD60x18 currentOpenInterestX18 = ud60x18(currentOpenInterest);
SD59x18 oldPositionSizeX18 = sd59x18(int256(oldPositionSize));
SD59x18 newPositionSizeX18 = sd59x18(int256(newPositionSize));
SD59x18 sizeDeltaX18 = newPositionSizeX18.sub(oldPositionSizeX18);
perpsEngine.exposed_checkOpenInterestLimits(
marketConfig.marketId,
sizeDeltaX18,
oldPositionSizeX18,
newPositionSizeX18
);
}
}
Traces:
[68701] PerpMarket_CheckOpenInterestLimits_Unit_Test::testFuzz_CheckOpenInterestLimits_NegativeNewOpenInterest(0, 0, 0, 0)
├─ [0] console::log(Bound result, 0) [staticcall]
│ └─ ← ()
├─ [0] console::log(Bound result, 1) [staticcall]
│ └─ ← ()
├─ [0] console::log(Bound result, 2) [staticcall]
│ └─ ← ()
├─ [0] console::log(Bound result, 0) [staticcall]
│ └─ ← ()
├─ [0] VM::assume(true) [staticcall]
│ └─ ← ()
├─ [0] VM::assume(true) [staticcall]
│ └─ ← ()
├─ [0] console::log(Bound result, 1) [staticcall]
│ └─ ← ()
├─ [11570] Perps Engine::exposed_checkOpenInterestLimits(1, -2, 2, 0) [staticcall]
│ ├─ [6311] PerpMarketHarness::exposed_checkOpenInterestLimits(1, -2, 2, 0) [delegatecall]
│ │ ├─ [0] console::log(Jai) [staticcall]
│ │ │ └─ ← ()
│ │ └─ ← "Arithmetic over/underflow"
│ └─ ← "Arithmetic over/underflow"
└─ ← "Arithmetic over/underflow"
Impact: High
Tools Use: Foundry
Recommendations:
The sub function used in Helpers.sol needs to be reviewed to handle potential overflow issues for the following reasons:
The checkOpenInterestLimits function is invoked in multiple scenarios, including:
1) The function is called in OrderBranch.sol, where there is a state change prior to invoking perpMarket.checkOpenInterestLimits.
perpMarket.checkOpenInterestLimits(
ctx.sizeDeltaX18, ctx.positionSizeX18, ctx.positionSizeX18.add(ctx.sizeDeltaX18)
);
2) The function is called in OrderBranch.sol, where there is a state change prior to invoking perpMarket.checkOpenInterestLimits.
(ctx.newOpenInterestX18, ctx.newSkewX18) =
perpMarket.checkOpenInterestLimits(sizeDeltaX18, ctx.oldPositionSizeX18, ctx.newPositionSizeX18);
Updates
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,87821 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.