RISE: RISEx Contracts
Cantina Security Report
Findings
High Risk
4 findings
4 fixed
0 acknowledged
Medium Risk
11 findings
11 fixed
0 acknowledged
Low Risk
20 findings
17 fixed
3 acknowledged
Informational
31 findings
24 fixed
7 acknowledged
High Risk4 findings
Function swapExactIn() skips pending maker settlement violating solvency invariant
Description
The function
swapExactIn()does not call_settlePendingForUser()before executing, unlikeplaceOrder(),cancelOrder(), andcancelOrders(), which all settle pending deferred maker fills first. This causes the swap to operate on staleavailable = balance - lockedvalues.When a user has pending (unsettled) deferred maker buy fills, their internal state is stale:
- balance has not been debited by claimQuoteAmount + makerFee
- locked has not been released by claimQuoteAmount
- Net overstatement of available = makerFee
The swap's executeHop computes
available = balance - lockedusing these stale values, allowing the hop to consume more quote than the user truly has available. After the swap completes and the deferred maker fill is eventually settled (via any subsequent placeOrder/cancelOrder call), updateBalanceByUserId debits the pending claimQuoteAmount + makerFee, driving the user's internal balance negative.Proof of Concept
The PoC demonstrates Alice ending with -0.01e18 QUOTE (negative by exactly the makerFee) after settlement.
/// @notice PoC: swapExactIn operates on stale `available = balance - locked` when the user /// has pending (unsettled) deferred maker buy fills. The makerFee portion has not been /// debited from balance yet, so `available` is overstated by exactly makerFee. /// After eventual settlement the user's internal balance goes negative. function test_audit_swap_budget_overstated_pending_maker_buy() external { // ── Setup: enable deferred mode & positive maker/taker fees (100 bps = 1%) ── vm.prank(address(spotManager)); ordersManager.setDeferredMode(marketId, true); feeManager.updateDefaultMakerFee(address(spotManager), int16(100)); feeManager.updateDefaultTakerFee(address(spotManager), int16(100)); // ── Step 1: Alice deposits exactly 1e18 QUOTE (just enough for 1-step buy) ── vm.startPrank(ALICE); quoteToken.approve(address(tokenManager), 1e18); tokenManager.deposit(ALICE, Currency.wrap(address(quoteToken)), 1e18); vm.stopPrank(); // ── Step 2: Alice places postOnly Buy limit at tick 1 (locks all 1e18 QUOTE) ── IOrdersManager.PlaceOrderParams memory params; params.marketId = marketId; params.sizeSteps = uint32(1); params.priceTicks = uint24(1); params.side = IOrdersManager.OrderSide.Buy; params.stpMode = IOrdersManager.STPMode.ExpireTaker; params.orderType = IOrdersManager.OrderType.Limit; params.postOnly = true; params.timeInForce = IOrdersManager.TimeInForce.GoodTillCancelled; params.headerVersion = uint8(1); vm.prank(ALICE); spotManager.placeOrder(params); assertEq(tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))), int256(1e18)); assertEq(spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18); // ── Step 3: Bob fills Alice's buy with a market sell (deferred → maker unsettled) ── baseToken.transfer(BOB, 1e18); vm.startPrank(BOB); baseToken.approve(address(tokenManager), 1e18); tokenManager.deposit(BOB, Currency.wrap(address(baseToken)), 1e18); vm.stopPrank(); params.side = IOrdersManager.OrderSide.Sell; params.orderType = IOrdersManager.OrderType.Market; params.postOnly = false; params.timeInForce = IOrdersManager.TimeInForce.ImmediateOrCancel; params.sizeSteps = uint32(1); vm.prank(BOB); spotManager.placeOrder(params); // Verify: Alice's state is STALE (maker settlement deferred) assertEq( tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))), int256(1e18), "stale: QUOTE balance unchanged" ); assertEq(spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18, "stale: locked unchanged"); // ── Step 4: CHARLIE places resting sell for the swap to fill against ── address CHARLIE = makeAddr("CHARLIE"); baseToken.transfer(CHARLIE, 10e18); vm.startPrank(CHARLIE); baseToken.approve(address(tokenManager), 10e18); tokenManager.deposit(CHARLIE, Currency.wrap(address(baseToken)), 10e18); vm.stopPrank(); params.side = IOrdersManager.OrderSide.Sell; params.orderType = IOrdersManager.OrderType.Limit; params.postOnly = true; params.timeInForce = IOrdersManager.TimeInForce.GoodTillCancelled; params.sizeSteps = uint32(5); params.priceTicks = uint24(1); vm.prank(CHARLIE); spotManager.placeOrder(params); // ── Step 5: Alice swaps QUOTE → BASE using fresh tokens ── // swapExactIn does NOT call _settlePendingForUser, so stale available is used address[] memory path = new address[](2); path[0] = address(quoteToken); path[1] = address(baseToken); vm.startPrank(ALICE); quoteToken.approve(address(spotManager), 1.1e18); spotManager.swapExactIn( ISpotManager.SwapExactInParams({ amountIn: 1.1e18, amountOutMin: 0, path: path, to: ALICE, deadline: block.timestamp + 1 }) ); vm.stopPrank(); // After swap: internal QUOTE balance ≈ 1e18, locked still 1e18 (stale) assertEq( spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18, "pre-settle: locked still stale" ); // ── Step 6: Trigger settlement via cancelOrders (empty array) ── RestingOrderId[] memory empty = new RestingOrderId[](0); vm.prank(ALICE); spotManager.cancelOrders(marketId, empty); // ── Step 7: Assert stale accounting caused a negative QUOTE balance ── int256 settledBalance = tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))); uint256 settledLocked = spotManager.getTotalLockedBalance(ALICE, address(quoteToken)); assertEq(settledLocked, 0, "settled: locked should be 0"); // makerFee = 1e18 * 100 / 10000 = 0.01e18 // The swap consumed all fresh tokens, then settlement debited (1e18 + makerFee) // and released locked (1e18), leaving balance negative by makerFee assertLt(settledBalance, 0, "settled: QUOTE balance should be negative (overstated budget)"); assertEq(settledBalance, -int256(0.01e18), "settled: negative by exactly makerFee"); }Impact Explanation
The issue has HIGH impact as it breaks the solvency invariant. User's internal balance goes negative, which the system assumes is impossible for swap participants. Moreover, the TokenManager holds fewer ERC20 tokens than the sum of all positive internal balances. The last user(s) to withdraw will be unable to redeem their full balance.
Likelihood Explanation
The likelihood of the issue is LOW - MEDIUM. It requires deferred settlement mode enabled on the market (production configuration for high-throughput markets) and positive maker fees (standard for most fee tiers). The issue does not require any special permissions, timing, or adversarial setup beyond normal trading activity
Recommendation
Call
_settlePendingForUser()for each market in the swap path before executing hops. This alignsswapExactIn()with the settlement pattern used by all other user-facing entry points.Moreover, adding a defensive check in
updateBalanceByUserId()to avoid negative balance will also prevent the issue.Cantina
The fix differs from recommendation. The actual fix uses projected reads instead of eager settlement. Both achieve correctness, but the projected-read approach is more gas-efficient (no storage writes) and doesn't require knowing which markets have pending fills upfront. No
_settlePendingForUser()was added. No defensiveupdateBalanceByUserId()check was added.PostOnly buy orders skip maker fee in solvency pre-check
Description
The function
placeOrder()inLibSpotOrderscomputes the required quote for buy orders as:uint256 feeAdjustment = params.postOnly ? 0 : _getTakerFeeAdjustment(...); uint256 requiredQuote = quoteAmount + feeAdjustment;The taker fee is correctly excluded for post-only orders, but the maker fee is also excluded. A user can place a post-only buy with exactly the available quote amount.
When filled, onMatch/onSettleMakerChunk debits
quoteAmount + makerFee, driving the balance negative. No solvency check exists for makers at fill time._requireQuoteSolvent()only runs for the taker's buy orders.PoC
The following PoC demonstrates the issue:
function test_audit_swap_budget_overstated_pending_maker_buy() external { // ── Setup: enable deferred mode & positive maker/taker fees (100 bps = 1%) ── vm.prank(address(spotManager)); ordersManager.setDeferredMode(marketId, true); feeManager.updateDefaultMakerFee(address(spotManager), int16(100)); feeManager.updateDefaultTakerFee(address(spotManager), int16(100)); // ── Step 1: Alice deposits exactly 1e18 QUOTE (just enough for 1-step buy) ── vm.startPrank(ALICE); quoteToken.approve(address(tokenManager), 1e18); tokenManager.deposit(ALICE, Currency.wrap(address(quoteToken)), 1e18); vm.stopPrank(); // ── Step 2: Alice places postOnly Buy limit at tick 1 (locks all 1e18 QUOTE) ── IOrdersManager.PlaceOrderParams memory params; params.marketId = marketId; params.sizeSteps = uint32(1); params.priceTicks = uint24(1); params.side = IOrdersManager.OrderSide.Buy; params.stpMode = IOrdersManager.STPMode.ExpireTaker; params.orderType = IOrdersManager.OrderType.Limit; params.postOnly = true; params.timeInForce = IOrdersManager.TimeInForce.GoodTillCancelled; params.headerVersion = uint8(1); vm.prank(ALICE); spotManager.placeOrder(params); assertEq(tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))), int256(1e18)); assertEq(spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18); // ── Step 3: Bob fills Alice's buy with a market sell (deferred → maker unsettled) ── baseToken.transfer(BOB, 1e18); vm.startPrank(BOB); baseToken.approve(address(tokenManager), 1e18); tokenManager.deposit(BOB, Currency.wrap(address(baseToken)), 1e18); vm.stopPrank(); params.side = IOrdersManager.OrderSide.Sell; params.orderType = IOrdersManager.OrderType.Market; params.postOnly = false; params.timeInForce = IOrdersManager.TimeInForce.ImmediateOrCancel; params.sizeSteps = uint32(1); vm.prank(BOB); spotManager.placeOrder(params); // Verify: Alice's state is STALE (maker settlement deferred) assertEq( tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))), int256(1e18), "stale: QUOTE balance unchanged" ); assertEq(spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18, "stale: locked unchanged"); // ── Step 4: CHARLIE places resting sell for the swap to fill against ── address CHARLIE = makeAddr("CHARLIE"); baseToken.transfer(CHARLIE, 10e18); vm.startPrank(CHARLIE); baseToken.approve(address(tokenManager), 10e18); tokenManager.deposit(CHARLIE, Currency.wrap(address(baseToken)), 10e18); vm.stopPrank(); params.side = IOrdersManager.OrderSide.Sell; params.orderType = IOrdersManager.OrderType.Limit; params.postOnly = true; params.timeInForce = IOrdersManager.TimeInForce.GoodTillCancelled; params.sizeSteps = uint32(5); params.priceTicks = uint24(1); vm.prank(CHARLIE); spotManager.placeOrder(params); // ── Step 5: Alice swaps QUOTE → BASE using fresh tokens ── // swapExactIn does NOT call _settlePendingForUser, so stale available is used address[] memory path = new address[](2); path[0] = address(quoteToken); path[1] = address(baseToken); vm.startPrank(ALICE); quoteToken.approve(address(spotManager), 1.1e18); spotManager.swapExactIn( ISpotManager.SwapExactInParams({ amountIn: 1.1e18, amountOutMin: 0, path: path, to: ALICE, deadline: block.timestamp + 1 }) ); vm.stopPrank(); // After swap: internal QUOTE balance ≈ 1e18, locked still 1e18 (stale) assertEq( spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18, "pre-settle: locked still stale" ); // ── Step 6: Trigger settlement via cancelOrders (empty array) ── RestingOrderId[] memory empty = new RestingOrderId[](0); vm.prank(ALICE); spotManager.cancelOrders(marketId, empty); // ── Step 7: Assert stale accounting caused negative QUOTE balance ── int256 settledBalance = tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))); uint256 settledLocked = spotManager.getTotalLockedBalance(ALICE, address(quoteToken)); assertEq(settledLocked, 0, "settled: locked should be 0"); // makerFee = 1e18 * 100 / 10000 = 0.01e18 // The swap consumed all fresh tokens, then settlement debited (1e18 + makerFee) // and released locked (1e18), leaving balance negative by makerFee assertLt(settledBalance, 0, "settled: QUOTE balance should be negative (overstated budget)"); assertEq(settledBalance, -int256(0.01e18), "settled: negative by exactly makerFee"); }Impact Explanation
The impact is HIGH as it can make the protocol insolvent. Either the protocol or the last users to withdraw will take the losses.
Likelihood Explanation
The issue has a MEDIM - HIGH likelihood.
- Requires only positive maker fees (standard configuration)
- Works on the default sync settlement path as there are no special mode needed
- Repeatable with no rate limiting
Recommendation
Include the maker fee in the pre-check while placing orders.
Cantina
PR #372 addressed the deferred projection issues (stale balance/locked reads in
withdraw()andswapExactIn()) but did not include the maker fee in the post-only buy pre-check. The maker fee fix was delivered separately in PR #475.Function withdraw() in TokenManager enables over-withdrawal due to stale available balance caused by deferred settlement
Description
The function
withdraw()inTokenManagercomputes available balance asbalance - lockedwithout settling pending deferred maker fills. In deferred settlement mode, when a maker BUY order is matched via onTakeLevel, the taker settles immediately, but the maker's balance and locked balance remain stale until onSettleMakerChunk is triggered by a settlement pump.Settlement would debit
matchedQuoteAmount + makerFeefrom the balance and release' matchedQuoteAmount' from locked, resulting in a net reduction of makerFee in the available balance.Since
withdraw()lives onTokenManager(a separate contract with no access to _settlePendingForUser), it reads the pre-settlement locked value directly from storage, overstating the available amount by exactly makerFee. The user can withdraw this excess before settlement runs.The root cause is identical to the
swapExactIn()finding: placeOrder, cancelOrder, and cancelOrders all call _settlePendingForUser is called before reading balances, butwithdraw()is not and structurally cannot be because it's on a different contract.Impact Explanation
The impact of the issue is High, as each over-withdrawal creates bad debt equal to the unsettled maker fee, making the protocol insolvent.
Likelihood Explanation
Likelihood for the issue is High as any maker with a filled but unsettled BUY order in deferred mode can call
withdraw()before the next settlement pump. No special timing or MEV required; the deferred settlement window persists until the user's next SpotManager interaction. An attacker can deliberately avoid triggering settlement after their order fills.Proof of Concept
The following PoC explains the issue in detail:
function test_audit_withdraw_stale_deferred_settlement() external { // ── Setup: enable deferred mode & positive maker fee (100 bps = 1%) ── vm.prank(address(spotManager)); ordersManager.setDeferredMode(marketId, true); feeManager.updateDefaultMakerFee(address(spotManager), int16(100)); feeManager.updateDefaultTakerFee(address(spotManager), int16(100)); // ── Step 1: Alice deposits 2e18 QUOTE (1e18 for order + 1e18 free) ── vm.startPrank(ALICE); quoteToken.approve(address(tokenManager), 2e18); tokenManager.deposit(ALICE, Currency.wrap(address(quoteToken)), 2e18); vm.stopPrank(); // ── Step 2: Alice places postOnly Buy for 1 step at tick 1 (locks 1e18 QUOTE) ── IOrdersManager.PlaceOrderParams memory params; params.marketId = marketId; params.sizeSteps = uint32(1); params.priceTicks = uint24(1); params.side = IOrdersManager.OrderSide.Buy; params.stpMode = IOrdersManager.STPMode.ExpireTaker; params.orderType = IOrdersManager.OrderType.Limit; params.postOnly = true; params.timeInForce = IOrdersManager.TimeInForce.GoodTillCancelled; params.headerVersion = uint8(1); vm.prank(ALICE); spotManager.placeOrder(params); // Confirm: balance=2e18, locked=1e18, available=1e18 assertEq(tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))), int256(2e18)); assertEq(spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18); // ── Step 3: Bob fills Alice's buy with a market sell (deferred → maker unsettled) ── baseToken.transfer(BOB, 1e18); vm.startPrank(BOB); baseToken.approve(address(tokenManager), 1e18); tokenManager.deposit(BOB, Currency.wrap(address(baseToken)), 1e18); vm.stopPrank(); params.side = IOrdersManager.OrderSide.Sell; params.orderType = IOrdersManager.OrderType.Market; params.postOnly = false; params.timeInForce = IOrdersManager.TimeInForce.ImmediateOrCancel; vm.prank(BOB); spotManager.placeOrder(params); // Verify state is STALE: balance=2e18, locked=1e18 (should be: balance=0.99e18, locked=0) assertEq( tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))), int256(2e18), "stale: balance unchanged" ); assertEq(spotManager.getTotalLockedBalance(ALICE, address(quoteToken)), 1e18, "stale: locked unchanged"); // ── Step 4: Alice withdraws using stale `available = balance - locked = 1e18` ── // True available after settlement would be: (2e18 - 1e18 - 0.01e18) - 0 = 0.99e18 // But stale available = 2e18 - 1e18 = 1e18 (overstated by makerFee = 0.01e18) vm.prank(ALICE); tokenManager.withdraw(ALICE, Currency.wrap(address(quoteToken)), 1e18); // ── Step 5: Trigger settlement ── RestingOrderId[] memory empty = new RestingOrderId[](0); vm.prank(ALICE); spotManager.cancelOrders(marketId, empty); // ── Step 6: Assert bad debt: Alice over-withdrew by makerFee ── int256 settledBalance = tokenManager.getBalance(ALICE, Currency.wrap(address(quoteToken))); uint256 settledLocked = spotManager.getTotalLockedBalance(ALICE, address(quoteToken)); assertEq(settledLocked, 0, "settled: locked should be 0"); // balance = (2e18 - 1e18 withdrawal) = 1e18 pre-settlement // settlement debits: 1e18 (matchedQuote) + 0.01e18 (makerFee) = 1.01e18 // settlement unlocks: 1e18 // final balance = 1e18 - 1.01e18 = -0.01e18 assertEq(settledBalance, -int256(0.01e18), "settled: negative by exactly makerFee (over-withdrawal)");}Recommendation
Add a settlement step before the available balance read in
withdraw().Cantina
Function
withdraw()doesn't call_settlePendingForUser(). Instead it readsgetProjectedBalanceByUserId()andgetTotalLockedBalanceByUserId()(which both go through LibSpotDeferredProjection), so the available balance calculation accounts for deferred fills without actually settling them.Wrong sign in _transferCollateral() funding materialization mints unbacked USDC balance on migration
Severity
- Severity: High
≈
Likelihood: High×
Impact: High Submitted by
Sujith S
Description
LibPerpsMigration._transferCollateral()adds pending funding fees to a user's balance instead of subtracting them. This creates a phantom balance on the old account after migration, minting unbacked value equal to the funding fee owed.When migrating,
_transferCollateral()materializes funding before transferring collateral :int256 crossFundingFee = LibPerpsCrossMarginView.totalCrossFundingFee(..., userId);if (crossFundingFee != 0) { s_registry.collateralManager .updateTokenBalanceByUserId(userId, Currency.wrap(LibConstant.USDC_TOKEN), crossFundingFee);}totalCrossFundingFee()returns positive when the user owes funding.updateTokenBalanceByUserId()doesbalance += delta. The transfer loop then callsgetTokenBalanceByUserId(), which already subtracts pending funding from the reported balance. So only the original balance is moved; the funding fees stay on the old account.Impact explanation
High, since the protocol creates value out of thin air and could lead to insolvency.
Likelihood explanation
High, its an user facing function. No special permissions required.
Recommendation
Materialization is unnecessary; positions already carry
lastFundingPayment, and the getter already deducts pending funding. Simply negating the sign does NOT work (causes double-charging, verified via PoC).function _transferCollateral(...) private {- int256 crossFundingFee = LibPerpsCrossMarginView.totalCrossFundingFee(...);- if (crossFundingFee != 0) {- s_registry.collateralManager- .updateTokenBalanceByUserId(userId, Currency.wrap(LibConstant.USDC_TOKEN), crossFundingFee);- } // Transfer all collateral tokens address[] memory m_tokens = s_registry.collateralManager.getSupportedCollateralTokens();
Medium Risk11 findings
USDC hardcoded at 1:1 valuation bypasses oracle and stablecoin peg monitoring during depeg events
Description
CollateralManager hardcodes USDC at a 1:1 USD valuation in three critical code paths, completely bypassing the oracle price system:
- Collateral valuation (
getTotalTokenBalanceInUSDByUserId()): USDC balances are added directly as USD without any price lookup.
if (tokenAddr != LibConstant.USDC_TOKEN) { uint256 price = oracle.getTokenPrice(tokenAddr); balance = balance.sMulWad(price.toInt256());}totalCollateralWad += balance;- Withdrawal health check (
_toUSDWithHealthCheck()): USDC amounts are returned as-is with no oracle lookup.
if (tokenAddr == LibConstant.USDC_TOKEN) return amountWad;- Oracle peg monitoring exclusion (
configureStablecoinPeg()): USDC is explicitly blocked from the on-chain stablecoin peg deviation system that protects other stablecoins via StablecoinDepegDetected reverts.
require(token != LibConstant.USDC_TOKEN, USDCRequiresExternalMonitoring());The Stork oracle adapter (RISExStork) supports arbitrary token price feeds via configurable priceId mappings, including USDC. The infrastructure to fetch a real-time USDC price already exists in the protocol's oracle stack, but CollateralManager bypasses it entirely with the hardcoded 1:1 assumption, and RISExOracle.configureStablecoinPeg explicitly blocks USDC from on-chain depeg monitoring.
During a USDC depeg event (e.g., USDC dropped to ~$0.87 in March 2023), the protocol continues to value USDC at $1.00. This enables the following attack path:
- Acquire depegged USDC on the open market at a discount (e.g., $0.90)
- Deposit into the protocol where it is valued at par ($1.00)
- Open maximum-leverage positions using the inflated collateral value
- Extract value by withdrawing other correctly-priced collateral tokens or profiting from positions sized beyond what the real collateral supports
The protocol becomes systematically insolvent: all USDC-denominated collateral is overvalued, liquidation thresholds are miscalculated, and the insurance fund is insufficient to cover the real deficit.
Likelihood Explanation
Though USDC depeg is a LOW likelihood, it is not theoretical and has occurred in practice. In March 2023, USDC lost its peg and traded at ~$0.87 following the collapse of Silicon Valley Bank. While USDC is generally considered stable, the protocol operates in a space where tail-risk events have outsized consequences.
Impact Explanation
USDC is the primary settlement and collateral token across the protocol. During a depeg, every USDC-denominated balance in the system is overvalued simultaneously. This affects all users and all markets, not a single position or single market. Leveraged positions are sized against inflated collateral, meaning liquidation thresholds are miscalculated system-wide. Hence, the impact is HIGH if such a scenario happens.
Recommendation
Since Stork already provides a USDC price feed, the integration cost is minimal. Configure a priceId for USDC in RISExStork, remove the USDCRequiresExternalMonitoring restriction in RISExOracle, and replace the hardcoded return amountWad early-returns in CollateralManager with actual oracle lookups.
- Collateral valuation (
Expired GoodTillTime orders remain matchable until epoch cleanup
Description
The matching engine in
LibMatchingEnginedoes not perform per-order expiry validation during order matching. When a GoodTillTime order's TTL expires, it remains on the book and is fully matchable until the epoch containing that order is cleaned.Order cleanup is epoch-based (ExpiryOrderCleaner), with epochs defined by a fixed interval (e.g., 60 minutes in the test configuration). A GTT order with a short TTL (e.g., 5 minutes) placed near the start of an epoch will remain matchable for up to the full epoch duration minus the TTL (55 minutes in this example) after its intended expiry.
The cleanup is triggered either by:
- An explicit call to
cleanExpiredEpochs()by a keeper -
- Automatic cleanup in
_requireEpochsCaughtUp()when the next order is placed
- Automatic cleanup in
Neither mechanism runs during matching itself. This means an expired order can be filled by an incoming taker order, because matching precedes any cleanup of the current epoch's orders.
A user placing a 5-minute GTT order reasonably expects it to be unmatchable after 5 minutes. Instead, it can be filled up to 55 minutes later (with a 60-minute epoch), fundamentally violating the expected semantics of time-limited orders.
PoC
The following Proof of Concept helps visualize the issue better:
function test_audit() external { vm.startPrank(ALICE); baseToken.approve(address(tokenManager), 100e18); tokenManager.deposit( ALICE, Currency.wrap(address(baseToken)), 100e18 ); vm.startPrank(BOB); quoteToken.approve(address(tokenManager), 100e18); tokenManager.deposit( BOB, Currency.wrap(address(quoteToken)), 100e18 ); IOrdersManager.PlaceOrderParams memory params; params.marketId = marketId; params.sizeSteps = uint32(1); params.priceTicks = uint24(1); params.ttlUnits = uint16(1); params.side = IOrdersManager.OrderSide.Buy; params.stpMode = IOrdersManager.STPMode.ExpireMaker; params.orderType = IOrdersManager.OrderType.Limit; params.postOnly = false; params.reduceOnly = true; params.timeInForce = IOrdersManager.TimeInForce.GoodTillTime; params.headerVersion = uint8(1); params.builderId = uint8(1); vm.startPrank(BOB); spotManager.placeOrder(params); vm.warp(block.timestamp + 10 minutes); vm.startPrank(ALICE); params.side = IOrdersManager.OrderSide.Sell; spotManager.placeOrder(params); vm.stopPrank(); }Likelihood explanation
This is not a conditional edge case; it occurs on every GTT order whose TTL is shorter than the epoch duration. Every epoch is planned to be configured to be 3 seconds, making the likelihood LOW.
Impact explanation
Impact is arguably High. Users who place short-lived GTT orders to limit exposure during volatile periods will have those orders filled well beyond their intended expiry. This is unintended protocol behavior.
Recommendation
Add a per-order expiry check in the matching engine's fill loop. When the matching engine encounters a resting order, it should compare the order's stored expiry timestamp against
block.timestamp. If the order has expired, skip it (or remove it inline) rather than filling it. This ensures that TTL semantics are enforced at match time, regardless of the epoch cleanup cadence.Cantina
Rather than adding per-order expiry checks in the matching hot path, the fix snaps GTT expiry timestamps to epoch boundaries at placement time, ensuring the existing epoch-based cleanup fires no later than the originally intended expiry. This eliminates the gap between raw expiry and epoch cleanup with zero gas overhead during matching. Trade-off: orders may expire up to one epoch earlier than the raw TTL requests.
- An explicit call to
Deferred maker settlement pays unbacked rebates when protocol fee pool is drained between taker and maker settlement
Description
In deferred settlement mode, order matching splits fee settlement into two phases:
onTakeLevel()(immediate taker settlement) andonSettleMakerChunk()(deferred maker settlement).When the maker fee is negative (rebate), the taker fee collected during
onTakeLevel()is temporarily held in totalFeesCollected and is meant to be deducted later duringonSettleMakerChunk()to fund the maker rebate.However, totalFeesCollected treats these temporarily held funds as withdrawable protocol revenue. An admin can call
withdrawTradingFees()between the two settlement phases and drain the pool. WhenonSettleMakerChunk()executes, it attempts to deduct the maker rebate from the now-empty pool via settleProtocolFee.The function caps the deduction at the available balance and returns the shortfall. But when no builder fee exists to absorb the shortfall, it is silently discarded:
uint256 makerBuilderFeeWad = _toWadUint(makerBuilderFee, quoteToken); // 0 when no buildermakerBuilderFeeWad = makerBuilderFeeWad > shortfall ? makerBuilderFeeWad - shortfall : 0; // still 0Meanwhile, the maker's balance was already unconditionally credited with the full rebate earlier in the same function:
// Maker-seller case: tokenMgr.updateBalanceByUserId(makerUserId, quoteToken, claimQuoteInt - makerFee); // claimQuoteInt - (-rebate) = claimQuoteInt + rebate → full rebate creditedThe result is that the maker receives the rebate in their balance, but the corresponding funds were never deducted from the fee pool, creating an untracked deficit that makes the TokenManager insolvent by the rebate amount.
The root cause is that
onTakeLevel()collects the full taker fee into totalFeesCollected even when part or all of it is passed through to the maker rebate, not actual protocol revenue.Proof of Concept
The following PoC explains the issue:
function test_deferredMakerRebateShortfall() public { address spotProxy = address(spotManager); Currency quoteC = Currency.wrap(address(quoteToken)); // 1. Configure fees: maker rebate (-1%) and taker fee (+1%) feeManager.updateDefaultMakerFee(spotProxy, int16(-100)); feeManager.updateDefaultTakerFee(spotProxy, int16(100)); // 2. Enable deferred settlement mode vm.prank(spotProxy); ordersManager.setDeferredMode(marketId, true); // 3. Bob places sell limit @ 4000 DAI (maker, will rest on book) IOrdersManager.PlaceOrderParams memory sellParams = _orderParams({ sizeSteps: 100, priceTicks: 40_000, side: IOrdersManager.OrderSide.Sell, orderType: IOrdersManager.OrderType.Limit }); vm.prank(BOB); spotManager.placeOrder(sellParams); // Record Bob's quote balance before settlement int256 bobQuoteBefore = tokenManager.getBalance(BOB, quoteC); // 4. Alice places buy limit that crosses (taker) // In deferred mode: taker settled immediately, maker deferred IOrdersManager.PlaceOrderParams memory buyParams = _orderParams({ sizeSteps: 100, priceTicks: 40_000, side: IOrdersManager.OrderSide.Buy, orderType: IOrdersManager.OrderType.Limit }); vm.prank(ALICE); spotManager.placeOrder(buyParams); // After taker settlement, fee pool has taker fee (1% of 4000 DAI = 40 DAI in WAD) uint256 feesAfterTaker = feeManager.getTotalFeesCollected(spotProxy, quoteC); assertGt(feesAfterTaker, 0, "Taker fees should be collected"); emit log_named_uint("Fee pool after taker settlement (WAD)", feesAfterTaker); // 5. Admin drains the fee pool quoteToken.mint(address(tokenManager), feesAfterTaker); feeManager.setProtocolFeeVault(spotProxy, address(tokenManager)); feeManager.withdrawTradingFees(spotProxy, address(tokenManager), quoteC, feesAfterTaker, ADMIN); uint256 feesAfterDrain = feeManager.getTotalFeesCollected(spotProxy, quoteC); assertEq(feesAfterDrain, 0, "Fee pool should be drained"); // 6. Pump deferred maker settlement // onSettleMakerChunk tries to deduct maker rebate from empty fee pool // Shortfall is silently dropped (no builder to absorb it) IPendingSettlementCleaner.PumpProgress memory progress = IPendingSettlementCleaner(address(ordersManager)) .pumpDirtyLevels({ protocol: spotProxy, marketId: marketId, maxLevels: 100, maxOrders: 100, maxSegments: 100 }); assertTrue(progress.done, "Settlement should complete"); assertGt(progress.processedOrders, 0, "Should have processed maker orders"); // 7. Verify the shortfall was silently dropped int256 bobQuoteAfter = tokenManager.getBalance(BOB, quoteC); int256 bobDelta = bobQuoteAfter - bobQuoteBefore; // Bob (maker-seller) should receive: quoteAmount - makerFee // = 4000e18 - (-40e18) = 4040e18 (rebate adds 40 DAI) // The maker received the full rebate even though the fee pool was empty assertGt(bobDelta, int256(4000e18), "Maker received rebate (more than quote amount)"); // Fee pool is 0, not negative -- shortfall silently dropped uint256 feesAfterSettle = feeManager.getTotalFeesCollected(spotProxy, quoteC); assertEq(feesAfterSettle, 0, "Fee pool is 0: shortfall silently dropped, not tracked"); emit log_named_int("Bob DAI delta (includes unbacked 40 DAI rebate)", bobDelta); emit log_named_uint("Fee pool after settlement (untracked deficit)", feesAfterSettle); }Impact Explanation
Medium - High. When the shortfall is dropped, the TokenManager becomes insolvent by the amount of the rebate. The deficit is not tracked anywhere; totalFeesCollected remains 0 rather than becoming negative.
Likelihood Explanation
Low. Three conditions must align simultaneously:
- Deferred settlement mode is enabled for the market
- Negative maker fees (rebates) are configured
- The protocol fee pool is drained between taker and maker settlement
Recommendation
Do not include the portion of the taker fee committed to maker rebates in totalFeesCollected. Two approaches:
-
Defer all fee settlement to onSettleMakerChunk: In onTakeLevel, settle only the taker's balance (quote and base token transfers), but do not call settleProtocolFee. Instead, defer the full fee netting (taker + maker) to onSettleMakerChunk, where both fee rates are known. This mirrors the sync path's netting semantics.
-
Track pending rebate obligations: Introduce a pendingRebateObligations field in the fee storage. In onTakeLevel, increment it by the expected maker rebate portion. In withdrawTradingFees, check totalFeesCollected - pendingRebateObligations instead of totalFeesCollected alone. In onSettleMakerChunk, decrement the obligation. This is more complex but preserves the current settlement structure.
Mark/index prices bypass deviation validation during single oracle source fail
Description
RISExOracle implements two price resolution paths with inconsistent security guarantees when one oracle source is unavailable:
-
Token prices (
_getTokenPriceWithFallback()) resolve the price from primary/fallback sources and then validate the result against a block EMA viaLibBlockEMA.validate(). This ensures that even when one oracle source is down, and the cross-source deviation check is skipped, a secondary manipulation guard remains active (EMA validation). -
Market prices (
_getPriceWithFallback()), used bygetIndexPrice()andgetMarkPrice(), perform a cross-source deviation check only when both sources succeed. When one source fails, the surviving source's price is returned with no secondary validation.
This creates an attack vector: if an adversary can cause one oracle source to revert or return zero (e.g., via stale-timestamp expiry, feed misconfiguration, or targeted griefing), the remaining source becomes fully trusted, with no manipulation guard.
The impact is amplified because
getIndexPrice()is a critical upstream dependency:- It anchors the mark price via getMarkPrice(), where two of the three median inputs (P1 = index + EMA premium, P2 = impact price) are derived from the index via
_computeInternalPrices(), so a corrupted index poisons the mark price. - It feeds directly into
FundingRate.snapshotAndSettleFunding()for funding rate settlement. - It is used in liquidation and margin calculations via the perps engine through CollateralManager.
A manipulated index price could trigger unfair liquidations, skew funding payments, or enable profitable position manipulation, all while bypassing the deviation safety net that token prices already enforce.
PoC
The following issue is demonstrated with the following test case:
/// @notice Full end-to-end PoC: manipulated index triggers unfair liquidation function test_unfairLiquidation_whenFallbackDown() public { // ═══════════════════════════════════════════════════════════════════ // Step 1: User opens a healthy SHORT position // ═══════════════════════════════════════════════════════════════════ // Deposit $10,000 USDC — plenty of margin for 0.5 BTC short at $100k // Initial margin required = 0.5 * $100,000 / 50x = $1,000 // Maintenance margin = 0.5 * $100,000 / 100 = $500 // Cross margin balance = $10,000 (very healthy, 20x above MM) _deposit(user, 10_000e6); IPerpsAccount.Position memory position = _createPosition(user, btcMarketId, IOrdersManager.OrderSide.Sell, 0.5e18, 100_000e18); emit log("--- Step 1: Position opened ---"); emit log_named_int(" Position size (WAD)", position.size); emit log_named_int(" Quote amount (WAD)", position.quoteAmount); // Verify position is healthy — margin balance >> maintenance margin int256 marginBefore = _getCrossMarginBalance(user); uint256 mmBefore = deployment.perpsProxy.getTotalCrossMaintenanceMargin(user); emit log_named_int(" Cross margin balance (WAD)", marginBefore); emit log_named_uint(" Maintenance margin (WAD)", mmBefore); assertTrue(marginBefore > int256(mmBefore), "Position should be healthy"); // ═══════════════════════════════════════════════════════════════════ // Step 2: Verify deviation check blocks manipulated prices normally // ═══════════════════════════════════════════════════════════════════ // Pump primary to $115k while fallback stays at $100k → 14% deviation > 5% threshold mockStork.setPriceNow(BTC_USD_INDEX_PRICE_ID, 115_000e18); mockStork.setPriceNow(BTC_USD_MARK_PRICE_ID, 115_000e18); vm.expectRevert(); deployment.risexOracleProxy.getIndexPrice(btcMarketId); emit log("--- Step 2: Deviation check blocks manipulated price (both sources live) ---"); // Restore honest price for next step mockStork.setPriceNow(BTC_USD_INDEX_PRICE_ID, 100_000e18); mockStork.setPriceNow(BTC_USD_MARK_PRICE_ID, 100_000e18); // ═══════════════════════════════════════════════════════════════════ // Step 3: Kill fallback oracle (simulates stale feed) // ═══════════════════════════════════════════════════════════════════ // Warp time forward by 2 hours — fallback prices become stale (threshold = 3600s) // Then set staleness threshold to 1 second so cached prices are definitely stale fallbackMockStork.setStalenessThreshold(1); vm.warp(block.timestamp + 2 hours); // Re-seed primary with fresh timestamps (primary stays alive) mockStork.setPriceNow(BTC_USD_INDEX_PRICE_ID, 100_000e18); mockStork.setPriceNow(BTC_USD_MARK_PRICE_ID, 100_000e18); // Also refresh USDC oracle to avoid stale issues elsewhere mockStork.setPriceNow(USDC_TOKEN_PRICE_ID, 1e18); emit log("--- Step 3: Fallback oracle killed (stale feed, threshold = 1s, age = 7200s) ---"); // ═══════════════════════════════════════════════════════════════════ // Step 4: Manipulate primary price — now passes UNCHECKED // ═══════════════════════════════════════════════════════════════════ // Pump BTC mark+index to $119,000 (19% above honest $100k) // For the SHORT position (0.5 BTC short, entry $100k): // unsettled = -0.5 * $119,000 + $50,000 = -$59,500 + $50,000 = -$9,500 loss // margin balance = $10,000 - $9,500 = $500 // maintenance margin = 0.5 * $119,000 / 100 = $595 // closeout threshold = $595 * 0.667 = $396.8 // $396 < $500 < $595 → LIQUIDATABLE (not RLP territory) // But at honest $100k: margin = $10,000, MM = $500 → very healthy! // // The 19% deviation would be caught by the 5% threshold if fallback was alive. mockStork.setPriceNow(BTC_USD_INDEX_PRICE_ID, 119_000e18); mockStork.setPriceNow(BTC_USD_MARK_PRICE_ID, 119_000e18); // Verify the manipulated price passes through unchecked uint256 manipulatedIndex = deployment.risexOracleProxy.getIndexPrice(btcMarketId); assertEq(manipulatedIndex, 119_000e18, "Manipulated price should pass unchecked"); emit log_named_uint(" Manipulated index price (WAD)", manipulatedIndex); // ═══════════════════════════════════════════════════════════════════ // Step 5: Position now appears liquidatable at the fake price // ═══════════════════════════════════════════════════════════════════ int256 marginAfter = _getCrossMarginBalance(user); uint256 mmAfter = deployment.perpsProxy.getTotalCrossMaintenanceMargin(user); emit log("--- Step 5: Position appears underwater at manipulated price ---"); emit log_named_int(" Cross margin balance (WAD)", marginAfter); emit log_named_uint(" Maintenance margin (WAD)", mmAfter); assertTrue(marginAfter < int256(mmAfter), "Position should appear liquidatable at fake price"); // ═══════════════════════════════════════════════════════════════════ // Step 6: Liquidator executes unfair liquidation // ═══════════════════════════════════════════════════════════════════ // Provide sell-side liquidity for the liquidation (closing a short = buy, needs sell orders) _placeLimitOrder( btcMarketId, liquidityProvider, 10e18, 119_000e18, IOrdersManager.OrderSide.Sell, true, false ); vm.prank(liquidator); deployment.perpsProxy.liquidate( IPerpsRiskManagement.LiquidateParams({ marketId: btcMarketId, account: user, expectedPrice: 125_000e18, expectedSize: 0.5e18, positionSide: IOrdersManager.OrderSide.Sell }) ); // Verify position is closed IPerpsAccount.Position memory positionAfter = deployment.perpsProxy.getPosition(btcMarketId, user); assertEq(positionAfter.size, 0, "Position should be liquidated"); emit log("--- Step 6: Liquidation succeeded ---"); emit log(" User's healthy position was unfairly liquidated using a manipulated price"); emit log(" The 19% price deviation bypassed all checks because the fallback oracle was down"); }Likelihood explanation
The issue has a very LOW likelihood, since for this to happen, either one of the oracles would need to fail and the other to be exploited.
Impact explanation
The impact is HIGH as this could result in potential loss of user funds.
Recommendation
Introduce a block EMA deviation check for market prices, mirroring the protection already applied to token prices:
- Add a
LibBlockEMA.EmaStatefield per market for index prices (e.g., in MarketOracleConfig or MarketMarkOracleState). - Update the index EMA in
syncMarkPriceEMA()(already called periodically) or introduce a dedicatedsyncIndexBlockEMA(uint16 marketId). - After
_getPriceWithFallback()resolves the index price ingetIndexPrice(), validate the result against the stored EMA, reverting if deviation exceeds a configurable per-market threshold.
This eliminates the inconsistency between the two price paths and ensures market prices receive the same secondary manipulation protection that token prices already have.
-
Session key operators can execute unlimited account actions via zero-amount permit
Description
In
PerpsRouter.sol, four account actions use the FLAG_PERMIT_SINGLE path with amount = 0:- _updateLeverage: _consumePermitSingle(account, msg.sender, Permission.Perps, 0)
- _updateMarginMode: _consumePermitSingle(account, msg.sender, Permission.Perps, 0)
- _updateIsolatedMargin: _consumePermitSingle(account, msg.sender, Permission.Perps, 0)
- _migrateAccount: _consumePermitSingle(account, msg.sender, Permission.Perps, 0)
_consumePermitSingle()callsconsumeAllowanceOnBehalf()onRISExAuthorization, which checks:require($.budget >= amount, InsufficientBudget(...)); $.budget -= uint96(amount);With amount = 0, the budget check trivially passes (budget >= 0 is always true), and the deduction is a no-op (budget -= 0). An operator with any non-zero session key budget can execute these actions an unlimited number of times without consuming any allowance.
Recommendation
Require per-operation signatures (FLAG_PERMIT with ECDSA/ERC-1271) for dangerous account actions, such as migration. Session keys should not be sufficient for account migration.
Or remove FLAG_PERMIT_SINGLE support entirely from
_migrateAccount(), and require a fresh cryptographic signature from the account owner for all migrations.Function getEffectivePosition() omits funding payment delta, causing incorrect cross-margin balance projections
Description
The function
getEffectivePosition()projects a user's effective position by applying pending deferred fills to the stored position. However, this projection only accounts for size and quote deltas from the pending fills, and it does not include the funding payment adjustment that actual settlement applies viaLibPerpsSettlement.settle().During real settlement, the quote amount is computed as:
int256 funding = LibPerpsMarginMath.calculateFundingPayment( position.size, position.lastFundingPayment, accumulatedFundingPayment); newQuote = matchedQuote + position.quoteAmount - funding - feesAmount;But in
_computePendingDeltas(), only pendingQuote and fee are considered:// Buy: quoteDelta -= int256(pendingQuote) + fee; // Sell: quoteDelta += int256(pendingQuote) - fee; // ← NO funding termThe
getDeferredPending()function only returns (pendingSteps, tick, feeBps) and does not expose fundingSnapshotX128, so the projection cannot compute the funding delta even if it tried. This discrepancy propagates toPerpsCrossMargin._marketDeferredDelta(), which uses getEffectivePosition to compute the PnL correction for cross-margin view functions (getCrossMarginBalance, getFreeCrossMarginBalance, getTotalCrossUnsettledUsdc).PoC
The following PoC explains the existence of the issue:
function test_getEffectivePosition_missingFundingDelta() public { // ===================================================================== // Step 1: Create LONG position for trader (1 BTC at $100k) // ===================================================================== _createPosition(trader, btcMarketId, IOrdersManager.OrderSide.Buy, 1e18, 100_000e18); _settleAllDeferred(btcMarketId); IPerpsAccount.Position memory posAfterCreate = deployment.perpsProxy.getPosition(btcMarketId, trader); emit log("--- Step 1: Position created ---"); emit log_named_int(" size (WAD)", posAfterCreate.size); emit log_named_int(" quoteAmount (WAD)", posAfterCreate.quoteAmount); emit log_named_int(" lastFundingPayment (WAD)", posAfterCreate.lastFundingPayment); // ===================================================================== // Step 2: Place resting buy orders above index to create positive premium // (longs pay shorts when bid impact > index) // ===================================================================== WideOrderId premiumBuyId = _placeLimitOrder(btcMarketId, premiumLP, 10e18, 102_000e18, IOrdersManager.OrderSide.Buy, true, false); // ===================================================================== // Step 3: Accumulate funding - snapshot premium & settle over 8 hours // ===================================================================== for (uint8 i; i < 8; ++i) { skip(1 hours); // Refresh oracle prices after time skip to avoid staleness mockStork.setPriceNow(BTC_USD_MARK_PRICE_ID, 100_000e18); mockStork.setPriceNow(BTC_USD_INDEX_PRICE_ID, 100_500e18); mockStork.setPriceNow(USDC_TOKEN_PRICE_ID, 1e18); deployment.fundingRateProxy.snapshotAndSettleFunding(btcMarketId); } (int128 accumulated,,) = deployment.fundingRateProxy.getFundingPayments(btcMarketId); emit log("--- Step 3: Funding accumulated ---"); emit log_named_int(" accumulatedFundingPayment (WAD)", int256(accumulated)); // Skip if funding didn't accumulate (would make test meaningless) assertTrue(accumulated != 0, "Funding should have accumulated"); // ===================================================================== // Step 4: Cancel premium orders so trader can place sell without crossing // ===================================================================== RestingOrderId[] memory idsToCancel = new RestingOrderId[](1); idsToCancel[0] = premiumBuyId.toRestingOrderId(); vm.prank(premiumLP); deployment.perpsProxy.cancelOrders(btcMarketId, idsToCancel); // ===================================================================== // Step 5: Trader places SELL maker at $100k (partial close 0.5 BTC) // ===================================================================== _placeLimitOrder(btcMarketId, trader, 0.5e18, 100_000e18, IOrdersManager.OrderSide.Sell, true, false); // ===================================================================== // Step 6: Crosser buys at $100k - crosses trader's sell (deferred fill) // Taker (crosser) settled immediately, maker (trader) deferred // ===================================================================== _placeLimitOrder(btcMarketId, crosser, 0.5e18, 100_000e18, IOrdersManager.OrderSide.Buy, false, false); // ===================================================================== // Step 7: Read PROJECTED position (getEffectivePosition - NO funding) // ===================================================================== IPerpsAccount.Position memory projectedPos = deployment.perpsProxy.getPosition(btcMarketId, trader); int256 projectedUnsettled = deployment.perpsProxy.getTotalCrossUnsettledUsdc(trader); emit log("--- Step 7: Projected position (before settlement) ---"); emit log_named_int(" projected size (WAD)", projectedPos.size); emit log_named_int(" projected quoteAmount (WAD)", projectedPos.quoteAmount); emit log_named_int(" projected unsettledUsdc (WAD)", projectedUnsettled); // ===================================================================== // Step 8: Settle deferred fills (applies funding via LibPerpsSettlement) // ===================================================================== _settleAllDeferred(btcMarketId); // ===================================================================== // Step 9: Read ACTUAL position (settled - WITH funding) // ===================================================================== IPerpsAccount.Position memory actualPos = deployment.perpsProxy.getPosition(btcMarketId, trader); int256 actualUnsettled = deployment.perpsProxy.getTotalCrossUnsettledUsdc(trader); emit log("--- Step 9: Actual position (after settlement) ---"); emit log_named_int(" actual size (WAD)", actualPos.size); emit log_named_int(" actual quoteAmount (WAD)", actualPos.quoteAmount); emit log_named_int(" actual unsettledUsdc (WAD)", actualUnsettled); // ===================================================================== // Step 10: Assert discrepancy - projected quoteAmount != actual quoteAmount // ===================================================================== int256 quoteDiscrepancy = projectedPos.quoteAmount - actualPos.quoteAmount; int256 unsettledDiscrepancy = projectedUnsettled - actualUnsettled; emit log("--- Step 10: Discrepancy ---"); emit log_named_int(" quoteAmount discrepancy (WAD)", quoteDiscrepancy); emit log_named_int(" unsettledUsdc discrepancy (WAD)", unsettledDiscrepancy); emit log_named_int(" lastFundingPayment (updated)", actualPos.lastFundingPayment); // Sizes must match (projection gets size right) assertEq(projectedPos.size, actualPos.size, "Sizes should match"); // quoteAmounts must differ (projection omits funding) assertNotEq( projectedPos.quoteAmount, actualPos.quoteAmount, "Projected quoteAmount should differ from actual - missing funding delta" ); // The discrepancy should be positive when longs owe funding (positive premium) // because the projection OVERESTIMATES quoteAmount by the funding amount assertTrue(quoteDiscrepancy > 0, "Projection overestimates quoteAmount (missing funding deduction)"); emit log("=== M-06: getEffectivePosition Missing Funding Delta ==="); emit log(" getEffectivePosition projects fills WITHOUT funding adjustment"); emit log(" getDeferredPending omits fundingSnapshotX128 - projection CANNOT compute funding"); emit log(" Affects: getPosition(), getTotalCrossUnsettledUsdc(), getCrossMarginBalance()"); }Impact Explanation
The issue has MEDIUM impact. View functions return wrong margin values. Off-chain systems (frontends, liquidation bots, risk engines) relying on getCrossMarginBalance or getFreeCrossMarginBalance will see incorrect figures. In some cases, if the funding delta is positive (user owes funding), the projected balance is overstated and liquidation bots may delay liquidation. If negative (user earns funding), the projected balance is understated triggering false liquidation signals.
Likelihood Explanation
The likelihood of the issue is MEDIUM. The condition exists whenever deferred mode is active, AND funding has accumulated since the user's last funding payment. Funding accumulates continuously on active markets (settled every hour), so any deferred fill that hasn't been pumped will have a stale lastFundingPayment.
Recommendation
Consider including the funding payment delta in
_computePendingDeltas().Cantina
The recommendation was to include the funding payment delta in
_computePendingDeltas()function. But the implemented fix deleted_computePendingDeltas()entirely and replaced the whole projection system, which is correct and comprehensive.Race condition on Permit approvals
Severity
- Severity: Medium
≈
Likelihood: Low×
Impact: High Submitted by
0xWeiss
Description
The
approveSinglefunction blindly overwrites the existing budget with the signed value without accounting for any remaining allowance.Because
consumeAllowancelets the operator spend the current budget at any time before the approval transaction executes, a malicious operator can drain the old budget and then submit the permit to reset the budget back to the new value.This creates the same race condition as the ERC20 approval pattern.
The result is that allowance updates are not atomic with their consumption, and an operator can spend both, the old and new budgets, even if the account intended the new permit to replace the previous allowance:
require( IUniversalSigValidator(LibConstant.UNIVERSAL_SIG_VALIDATOR).isValidSig(account, digest, signature), IPermitWitness.InvalidSignature(account) ); // Write budget and allowanceExpiry into the shared Details slot ISignerManager.Details storage $ = s_sessionKeys[account][operator];>> $.budget = budget; $.allowanceExpiry = allowanceExpiry; emit IPermitSingle.Approval(account, operator, budget, allowanceExpiry); }Recommendation
Add a couple checks, either check that the permit has been fully cleared by calling
lockdownAllowance, never created, or expired:require(budget > 0, IPermitSingle.InvalidBudget()); require(allowanceExpiry > block.timestamp, ISignerManager.InvalidExpiration(allowanceExpiry)); + ISignerManager.Details storage $ = s_sessionKeys[account][operator];+ require(+ ($.budget == 0 && $.allowanceExpiry == 0) || $.allowanceExpiry < block.timestamp,+ IPermitSingle.InvalidBudget()+ );+ // Consume nonce from the shared anchored bitmap _useNonce(account, nonceAnchor, nonceBitmap); ); bytes32 digest = _hashTypedDataV4( keccak256( abi.encode( LibTypehash.PERMIT_SINGLE, account, operator, budget, allowanceExpiry, nonceAnchor, nonceBitmap ) ) ); require( IUniversalSigValidator(LibConstant.UNIVERSAL_SIG_VALIDATOR).isValidSig(account, digest, signature), IPermitWitness.InvalidSignature(account) ); // Write budget and allowanceExpiry into the shared Details slot- ISignerManager.Details storage $ = s_sessionKeys[account][operator]; $.budget = budget; $.allowanceExpiry = allowanceExpiry;Unexpected oracle reverts can freeze withdrawals
Description
In the
_executeWithdrawfunction, the withdrawal flow calls_toUSDWithHealthCheckfor the specific token being withdrawn.Immediately after, the
getWithdrawableUSDfunction is called, which computes the total withdrawable amount by iterating all supported tokens and callingoracle.getTokenPrice.if (_isEmergencyActive()) { _queuePendingWithdrawal(sender, token, amount); return; } // Check withdrawable margin (in USD) — only reached when oracle is healthy {>> uint256 withdrawableUSD = getWithdrawableUSD(sender); require( withdrawableUSD >= amountInUSD, LibErrors.InsufficientBalance(sender, tokenAddr, withdrawableUSD, amountInUSD) ); }If any token in the user’s portfolio has a failing oracle (deviation, or any other revert reason), the call to
_executeWithdrawreverts and the entire withdrawal will fail.Recommendation
Make sure if an oracle of any of the supported assets reverts, it is handled appropriately:
diff --git a/src/protocols/exchanges/perps/collateral/CollateralManager.sol b/src/protocols/exchanges/perps/collateral/CollateralManager.sol--- a/src/protocols/exchanges/perps/collateral/CollateralManager.sol+++ b/src/protocols/exchanges/perps/collateral/CollateralManager.sol@@ -287,7 +287,18 @@ // Re-check withdrawable margin with current prices {- uint256 withdrawableUSD = getWithdrawableUSD(account);+ uint256 withdrawableUSD;+ try this.getWithdrawableUSD(account) returns (uint256 value) {+ withdrawableUSD = value;+ } catch (bytes memory reason) {+ if (_isDeviationError(reason)) {+ _triggerEmergency(tokenAddr, 0, "ORACLE_DEVIATION", address(this));+ _requireNotEmergencyAfterHealthCheck();+ }+ // Re-revert non-deviation errors+ assembly ("memory-safe") {+ revert(add(reason, 0x20), mload(reason))+ }+ } require( withdrawableUSD >= amountInUSD, LibErrors.InsufficientBalance(account, tokenAddr, withdrawableUSD, amountInUSD) diff --git a/src/protocols/exchanges/perps/collateral/CollateralManager.sol b/src/protocols/exchanges/perps/collateral/CollateralManager.sol--- a/src/protocols/exchanges/perps/collateral/CollateralManager.sol+++ b/src/protocols/exchanges/perps/collateral/CollateralManager.sol@@ -340,7 +340,19 @@ // Check withdrawable margin (in USD) — only reached when oracle is healthy {- uint256 withdrawableUSD = getWithdrawableUSD(sender);+ uint256 withdrawableUSD;+ try this.getWithdrawableUSD(sender) returns (uint256 value) {+ withdrawableUSD = value;+ } catch (bytes memory reason) {+ if (_isDeviationError(reason)) {+ _triggerEmergency(tokenAddr, 0, "ORACLE_DEVIATION", address(this));+ _queuePendingWithdrawal(sender, token, amount);+ return;+ }+ // Re-revert non-deviation errors+ assembly ("memory-safe") {+ revert(add(reason, 0x20), mload(reason))+ }+ } require( withdrawableUSD >= amountInUSD, LibErrors.InsufficientBalance(sender, tokenAddr, withdrawableUSD, amountInUSD)Cantina
Instead of wrapping
getWithdrawableUSD()in try/catch at withdrawal call sites, the project made the underlying collateral valuation function itself oracle-failure-resilient as positive balances silently zero out on oracle failure while negative balances (debt) still hard-revert to prevent equity inflation.Missed keeper intervals permanently reduce accumulated funding
Description
The
settleFundingRate()inFundingRatemakes exactly one funding payment per invocation, regardless of how much wall-clock time has elapsed since the previous settlement. After each settlement, lastUpdatedAt is set to theblock.timestamp, not tolastTimestamp + SETTLE_FUNDING_RATE_INTERVAL. This means the next settlement cannot occur until a full hour has elapsed since the current block, and there is no mechanism to "catch up" for missed intervals.The funding payment amount is computed as:
int128 newFundingPayment = fundingRate_.sMulWad(indexPrice.toInt256()).toInt128();This value is independent of the time elapsed. Whether 1 hour or 10 hours have passed, the increment to fundingPayments[marketId].accumulated is the same single payment.
Because per-position funding is calculated as (accumulated - position.lastFundingPayment) * size, any gap in the accumulated value directly translates to funding that positions never pay or receive. The deficit is permanent, once LastUpdatedAt advances; the skipped intervals cannot be reclaimed.
PoC
Over a 5-hour window with identical market conditions (constant premium index, same funding rate per settlement):
/// @notice PoC: 5-hour window - normal keeper (5 settlements) vs keeper with 4-hour gap (2 settlements) /// @dev Uses vm.snapshot/revertTo to run both paths from identical starting state function test_missedSettlementsPermanentlyReduceAccumulated() public { uint256 snap = vm.snapshot(); // ===================================================================== // PATH A: Normal keeper - settles every hour for 5 hours // ===================================================================== for (uint256 i; i < 5; ++i) { _fundOneHour(); } (int128 normalAccumulated, int96 normalRate,) = deployment.fundingRateProxy.getFundingPayments(btcMarketId); emit log("=== PATH A: Normal keeper (5 hourly settlements) ==="); emit log_named_int(" accumulated (WAD)", int256(normalAccumulated)); emit log_named_int(" currentFundingRate (WAD)", int256(normalRate)); // ===================================================================== // PATH B: Keeper with 4-hour gap (revert to same starting state) // ===================================================================== vm.revertTo(snap); // Hour 1: normal settlement _fundOneHour(); (int128 afterHour1,,) = deployment.fundingRateProxy.getFundingPayments(btcMarketId); // Hours 2-5: keeper offline - no snapshots, no settlements skip(4 hours); _refreshBtcPrices(); // Keeper returns: must snapshot first (pendingCount was 0 after last settle) deployment.fundingRateProxy.snapshotPremiumIndex(btcMarketId); // Now settle (pendingCount=1, interval well exceeded at 14400s > 3600s) deployment.fundingRateProxy.settleFundingRate(btcMarketId); (int128 gapAccumulated, int96 gapRate,) = deployment.fundingRateProxy.getFundingPayments(btcMarketId); emit log("=== PATH B: Keeper with 4-hour gap (2 settlements) ==="); emit log_named_int(" accumulated after hour 1 (WAD)", int256(afterHour1)); emit log_named_int(" accumulated after gap settle (WAD)", int256(gapAccumulated)); emit log_named_int(" currentFundingRate (WAD)", int256(gapRate)); // ===================================================================== // IMPACT: Compute funding loss // ===================================================================== int256 lostFunding = int256(normalAccumulated) - int256(gapAccumulated); // Position was created with lastFundingPayment = 0 (no prior funding) // For 1 BTC (size=1e18), pending funding = accumulated * 1e18 / 1e18 = accumulated IPerpsAccount.Position memory pos = deployment.perpsProxy.getPosition(btcMarketId, longTrader); int256 normalPending = int256(normalAccumulated - pos.lastFundingPayment).sMulWad(pos.size); int256 gapPending = int256(gapAccumulated - pos.lastFundingPayment).sMulWad(pos.size); emit log("=== IMPACT ==="); emit log_named_int(" Lost accumulated funding per BTC (WAD)", lostFunding); emit log_named_int(" Trader pending funding - normal (WAD)", normalPending); emit log_named_int(" Trader pending funding - gap (WAD)", gapPending); emit log_named_int(" Trader underpayment (WAD)", normalPending - gapPending); // Normal: 5 settlements. Gap: 2 settlements. Each charges the same rate. // Expected loss: 3/5 = 60% uint256 lossPercent = uint256(lostFunding) * 100 / uint256(int256(normalAccumulated)); emit log_named_uint(" Loss percentage", lossPercent); // ===================================================================== // ASSERTIONS // ===================================================================== // Gap path must accumulate strictly less funding than normal assertGt( int256(normalAccumulated), int256(gapAccumulated), "Normal path must accumulate more funding than gap path" ); // Loss must exceed 50% (3 of 5 settlements missed) assertGt(lossPercent, 50, "Funding loss must exceed 50%"); // Per-settlement funding rate is identical in both paths (constant prices -> same premium) // This proves the issue is the NUMBER of settlements, not the rate computation assertEq(normalRate, gapRate, "Per-settlement funding rate should be identical"); emit log("=== CONCLUSION ==="); emit log(" settleFundingRate() charges ONE payment per call regardless of time elapsed."); emit log(" lastUpdatedAt is set to block.timestamp, not lastTimestamp + interval."); emit log(" Missed settlement intervals are permanently lost - no catch-up mechanism."); }- Per-settlement funding rate is identical in both paths (1.803e15), confirming the issue is settlement count, not rate computation.
- Lost funding per BTC: ~$543.66 (66% underpayment)
- After the gap settlement, an immediate retry is blocked (accumulated unchanged), confirming no catch-up path exists.
Impact Explanation
High. The side of the market that should be receiving funding permanently loses payments for every missed interval. For the side that should be paying, this is an equivalent windfall; they hold leveraged exposure without paying the full cost of carry. The funding loss is not recoverable through any on-chain mechanism.
Likelihood Explanation
Low. Though heavily controlled, keeper downtime is a realistic operational scenario on any chain:
- Keeper infrastructure failures (cloud provider issues, misconfiguration, key rotation).
- Network congestion during high-volatility events: precisely when funding rates matter most and positions are largest.
The protocol relies on an external keeper calling snapshotAndSettleFunding every hour. Any disruption to this single dependency triggers the bug. Additionally, because snapshotPremiumIndex shares the same keeper path, a gap also means zero premium snapshots accumulate, so the first post-gap settlement uses a sparse TWAP (1-2 data points instead of ~720), amplifying inaccuracy.
Recommendation
Set lastUpdatedAt to advance by exactly one interval rather than jumping to the current timestamp, allowing repeated calls to catch up one period at a time:
s_fundingRate.fundingPayments[marketId].lastUpdatedAt = (lastTimestamp + LibConstant.SETTLE_FUNDING_RATE_INTERVAL).toUint32();This preserves the one-payment-per-call semantics but allows the keeper (or a caller) to invoke settleFundingRate in a loop to process each missed interval sequentially. Each iteration uses the snapshots that were available during that window.
For gaps where no snapshots were taken (keeper fully offline), consider carrying forward the previous settlement's funding rate as a fallback rather than requiring fresh pendingCount > 0. This ensures funding accrues continuously even during outages, consistent with how centralized exchanges handle scheduled funding at fixed intervals regardless of operational state.
Cantina
The second part of the recommendation (carry forward previous funding rate when no snapshots exist) was not implemented. The fix still requires
pendingCount > 0for each settlement. So the fix is partial: it handles "keeper came back and can snapshot + settle in a loop" but not "keeper was fully offline and no snapshots were taken during the gap."Liquidation fee not accumulated to protocol revenue
Description
In
LibPerpsHooks.sol, when a liquidation order is matched, the liquidation fee is correctly computed viaLibPerpsMarginMath.calculateFees()and deducted from the liquidated trader's PnL during position settlement. However, the fee is never passed toLibPerpsTransientCache.accumulateFees(), so it is excluded from the protocol's fee pool at flush time.The bug exists in both matching paths:
// onTakeLevel (deferred mode, line 338-345): (, int256 takerFee, int256 liquidationFee) = LibPerpsMarginMath.calculateFees(0, takerCtx.takerFeeBps, takeCtx.matchedQuoteAmount, isLiquidation); takerSettlementFees = takerFee + liquidationFee; // Both deducted from trader PnLif (takerFee != 0) { LibPerpsTransientCache.accumulateFees(takerFee); // Only takerFee accumulated!} // onMatch (sync mode, line 207-209): if (m_fees.taker != 0) { LibPerpsTransientCache.accumulateFees(m_fees.taker); }In both paths,
takerSettlementFees = takerFee + liquidationFeeis passed to the settlement function, which subtracts it from the trader's balance viaupdateTokenBalanceByUserId(). But at flush time (_flushAccumulatedSettlement()) accumulatedFees contains only the taker-fee portion. The protocol calls collectFees with this amount, permanently excluding the liquidation fee from the protocol's revenue ledger.The liquidation fee remains as an unaccounted surplus in the CollateralManager contract, inaccessible through normal fee withdrawal mechanisms.
PoC
function test_liquidationFeeNotCollectedByProtocol() public { // Create a short position that is immediately liquidatable // Entry at $99,019 when mark is $100k creates immediate unrealized loss IPerpsAccount.Position memory position = _createPosition(trader, btcMarketId, IOrdersManager.OrderSide.Sell, 0.1e18, 99_019e18); _settleAllDeferred(btcMarketId); // Sanity: verify position state matches expected values assertEq(position.isolatedUsdcBalance, 198.038e18, "Isolated balance should be $198.038"); // Place counterparty sell orders for the liquidation buy order to match against _placeLimitOrder( btcMarketId, liquidityProvider, 10e18, 100_000e18, IOrdersManager.OrderSide.Sell, true, false ); // Record state BEFORE liquidation int256 traderBalanceBefore = deployment.collateralManagerProxy.getTotalTokenBalanceInUSD(trader); uint256 protocolFeesBefore = deployment.feeManagerProxy.getTotalFeesCollected(address(deployment.perpsProxy), usdcCurrency); // Execute liquidation vm.prank(liquidator); deployment.perpsProxy.liquidate( IPerpsRiskManagement.LiquidateParams({ marketId: btcMarketId, account: trader, expectedPrice: 105_000e18, expectedSize: 0.1e18, positionSide: IOrdersManager.OrderSide.Sell }) ); _settleAllDeferred(btcMarketId); // Record state AFTER liquidation int256 traderBalanceAfter = deployment.collateralManagerProxy.getTotalTokenBalanceInUSD(trader); uint256 protocolFeesAfter = deployment.feeManagerProxy.getTotalFeesCollected(address(deployment.perpsProxy), usdcCurrency); uint256 feesCollectedFromLiquidation = protocolFeesAfter - protocolFeesBefore; // 1. Trader DID pay the liquidation fee (balance decreased by PnL + liq fee) // Expected: $1000 - $98.1 (PnL loss) - $100 (liq fee) = $801.9 assertEq(traderBalanceAfter, 801.9e18, "Trader paid PnL loss + liquidation fee"); // 2. Protocol collected ZERO fees from the liquidation assertEq(feesCollectedFromLiquidation, 0, "BUG: Protocol collected ZERO fees from liquidation"); }Likelihood Explanation
High. The bug triggers on every liquidation without exception. No special conditions, timing, or attacker behavior is required.
Impact Explanation
Low. The protocol permanently loses 1% of matched notional (LIQUIDATION_FEE_BPS = 100) on every liquidation. However, the token can be recovered through an upgrade.
Recommendation
Consider accumulating the liquidation fee alongside the taker fee in both matching paths.
Privilege escalation in Permission.All checks of _hasPermission() function
Description
The function
_hasPermission()inAuthorizationBase.solusesbitmap & mask != 0to check permissions.Permission.Allis encoded astype(uint32).max(0xFFFFFFFF), so the checkbitmap & 0xFFFFFFFF != 0returns true for any non-zero bitmap.A signer granted only a single permission (e.g.
Permission.Perps) incorrectly passes thePermission.Allcheck, appearing to have full privileges. This means any code path that gates onPermission.Allto verify a signer has unrestricted access can be bypassed by a signer with any single permission.Recommendation
Replace != 0 with == _encodePermissionBitmap(permission) so that all required bits must be set:
- return s_sessionKeys[userId][signer].permissionBitmap & _encodePermissionBitmap(permission) != 0;+ return s_sessionKeys[userId][signer].permissionBitmap & _encodePermissionBitmap(permission)+ == _encodePermissionBitmap(permission);
Low Risk20 findings
ERC-7201 storage slot constants do not match documented namespace strings
Description
Three ERC-7201 namespaced storage location constants contain hardcoded bytes32 values that do not correspond to the ERC-7201 derivation
(keccak256(abi.encode(uint256(keccak256(namespace)) - 1)) & ~bytes32(uint256(0xff)))of their documented namespace strings.File Namespace (documented) Hardcoded Correct (computed) MarketBookStorage.sol risex.storage.orderbook.MarketBook 0xf816...e500 0x5350...9900 CollateralGuardStorage.sol risex.storage.collateral.Guard 0xb5e3...1e00 0x4380...0600 TokenStorage.sol risex.storage.shared.token.Token 0x1712...0900 0x9225...4f00 While the contracts will function correctly regardless (the hardcoded slot is what actually gets used at runtime), this discrepancy has the following consequences:
-
Tooling incompatibility: Off-chain tools that rely on the @custom:storage-location erc7201: annotation (e.g., OpenZeppelin Upgrades plugin, storage layout validators, block explorers) will compute a different slot than the one actually used, making automated storage verification and upgrade safety checks unreliable.
-
Misleading documentation: The NatSpec comments are factually incorrect and could mislead auditors or developers verifying storage safety.
Recommendation
Recompute the constants from the documented namespace strings using cast index-erc7201 and update the hardcoded values.
-
Trusted forwarder stored as immutable in PerpsManager and SpotManager
State
- Acknowledged
Severity
- Severity: Low
Submitted by
Sujith S
Description
Both
PerpsManagerandSpotManagerinheritERC2771ContextUpgradeable, which stores the trusted forwarder as an immutable variable set via the constructor:// OpenZeppelin ERC2771ContextUpgradeableaddress private immutable _trustedForwarder; constructor(address trustedForwarder_) { _trustedForwarder = trustedForwarder_; }Since these are upgradeable proxy contracts, immutable values are embedded in the implementation bytecode rather than in proxy storage. This creates two problems:
-
Upgrade fragility: Every new implementation deployment must pass the exact same trusted forwarder address to the constructor. If a different address is passed (or the parameter is overlooked), the forwarder silently changes for all users behind the proxy, potentially breaking meta-transaction flows or, worse, granting forwarder privileges to an unintended address.
-
No on-chain update path: If the trusted forwarder needs to be rotated (e.g., the forwarder contract is compromised or migrated), the only option is deploying an entirely new implementation as there is no governance-callable setter.
Recommendation
Override
trustedForwarder()in bothPerpsManagerandSpotManagerto read from ERC-7201 namespaced storage instead of the immutable.Rise Labs
This is by design, the trusted forwarder is the UniversalRouter, which is itself an upgradeable proxy, so its address never changes. Rotation is handled by upgrading the router's implementation, not by changing the forwarder address. The immutable approach avoids an extra storage read on every meta-transaction call.
Function transferTradingFees() can drain user deposits
Description
The function
transferTradingFees()inTokenManager.solis guarded only by the restricted modifier (AccessManager role) and validates the requested amount against the contract's raw IERC20.balanceOf(address(this)), which includes both user deposit balances and accumulated trading fees:uint256 tokenBalance = IERC20(tokenAddr).balanceOf(address(this));require(tokenBalance >= amount, LibErrors.InsufficientSupply(tokenAddr, tokenBalance, amount));IERC20(tokenAddr).safeTransfer(to, amount);While the intended call path is through
FeeManager.withdrawTradingFees(), which enforcesamount <= totalFeesCollected[token]before delegating to transferTradingFees, the function itself is independently callable by any address holding the appropriate AccessManager role.A compromised or malicious privileged account can bypass FeeManager's accounting and withdraw the entire token balance, including user deposits. The same issue exists in
CollateralManager.sol.Recommendation
Replace the restricted modifier with an explicit caller check limiting invocation to the FeeManager contract only, ensuring all withdrawals go through FeeManager's fee accounting.
Functions pauseFunction and unpauseFunction can bypass global pause
Severity
- Severity: Low
Submitted by
Sujith S
Description
The function
_requireValidPauseSelector()inPartialPauseManagedUpgradeable.solexplicitly blockspauseFunction.selectorandunpauseFunction.selectorfrom the per-function pause mapping, expressing the design intent that these functions should never be pausable. However, neither function carries thewhenNotPausedmodifier:function pauseFunction(bytes4 selector) external restricted { _pauseFunction(selector);} function unpauseFunction(bytes4 selector) external restricted { _unpauseFunction(selector);}Both functions remain callable even when the contract is globally paused. This becomes a privilege boundary violation since a lower-privilege role can alter the system's safety configuration during a global emergency triggered by a higher-privilege role.
Recommendation
Add
whenNotPausedto bothpauseFunction()andunpauseFunction()so they respect the global pause state.Rounding in LibMatchingEngine systematically favors taker over maker
Description
LibMatchingEngine computes the matched quote amount using mulWad (rounds down) in both the sync and deferred fill paths:
Sync path (_matchNextMaker):uint128 quoteAmount = lvl.levelPrice.mulWad(m.matched).toUint128(); Deferred path (_invokeTakeLevelHook):matchedQuoteAmount: levelPrice.mulWad(matched).toUint128()The function
mulWad()performs floor(price * size / 1e18), so the quote amount is always rounded down. This single rounded value is used for both sides of the settlement inLibSpotHooksandLibPerpsHooks:- Taker buys (maker sells): taker pays matchedQuoteInt (rounded down and pays less), maker receives matchedQuoteInt (rounded down, receives less than exact price * size). Rounding favors the taker.
- Taker sells (maker buys): taker receives matchedQuoteInt (rounded down and receives less), maker pays matchedQuoteInt (rounded down, pays less). Rounding favors the maker.
The maker is the passive party who posted a resting order at a specific price expecting
price * sizein quote. When the maker is a seller,mulWad()rounding consistently reduces the quote they receive; the maker absorbs the rounding loss on every fill. At high-frequency, small-size fills, this dust accumulates against sell-side liquidity providers, creating a systematic micro-disadvantage for makers providing ask-side liquidity.Standard exchange convention rounds in favor of the maker (liquidity provider) to incentivize order book depth. The current implementation violates this by applying a uniform round-down regardless of which party benefits.
Recommendation
Use directional rounding based on who should absorb the dust — the taker (aggressor) should always bear the rounding loss, and the maker (liquidity provider) should always benefit:
// When taker is buying (maker is selling): round UP so maker receives more// When taker is selling (maker is buying): round DOWN so maker pays lessuint128 quoteAmount = isTakerBuy ? lvl.levelPrice.mulWadUp(m.matched).toUint128() : lvl.levelPrice.mulWad(m.matched).toUint128();This ensures rounding dust always favors the maker, consistent with standard exchange design conventions that incentivize liquidity provision.
Unused settlement bound constants in SpotManagerBase
Description
The contract
SpotManagerBasedefines three settlement-bound constants: PENDING_SETTLE_MAX_LEVELS, PENDING_SETTLE_MAX_ORDERS, and PENDING_SETTLE_MAX_SEGMENTS, but never uses them.The
_settlePendingForUser()function callspumpForUser()in the order book, which does not accept any of these caps. Without enforced bounds, a single_settlePendingForUser()call could consume unbounded gas if a user has many pending settlements, potentially causing transactions to revert due to gas limits.Recommendation
Either remove the dead constants or consider implementing a bounded pump variant that accepts maxLevels, maxOrders, and maxSegments as caps.
Missing maxOrderSize >= minOrderSize validation in LibSpotMarket.update
Description
The function
add()inLibSpotMarketvalidatesparams.config.maxOrderSize >= params.config.minOrderSizewhen creating a market, but theupdate()function performs no such check when updating market config.An admin could inadvertently set
minOrderSize > maxOrderSize, making the market unusable asLibSpotOrdersenforces bothsize >= minOrderSizeandsize <= maxOrderSize, so no valid order size would exist.Recommendation
Add the same validation to update:
require( config.maxOrderSize >= config.minOrderSize, LibErrors.InvalidMaxOrderSize(config.maxOrderSize, config.minOrderSize));Market lock does not prevent settlement
Description
Functions
pumpForUser()andpumpDirtyLevels()onOrdersManagerare permissionless externals (nonReentrant
whenNotPaused, no access control). They call back into SpotManager's onSettleMakerChunk hook, which only checksonlyOrdersManager, not market lock.This means anyone can trigger settlement for any user on a locked market, mutating balances, locked amounts, and fee accruals despite the admin's intent to freeze state.
SpotManager enforces onlyUnlockedMarket on all user-facing entry points (placeOrder, cancelOrder, cancelOrders), but the settlement hooks (onMatch, onTakeLevel, onSettleMakerChunk, onAddToBook, onRemoveFromBook) have no equivalent guard. The lock is therefore only a partial freeze, blocking new order flow but not pending settlement.
Recommendation
Either check the market lock inside the settlement hooks (onSettleMakerChunk, etc.), or if settlement during a lock is intentionally allowed, document this behavior explicitly so admins understand the lock only prevents new order flow, not pending settlement and handle security incidents in a case-by-case basis.
Maker fee rebate configuration not validated against taker fee during setup
Severity
- Severity: Low
Submitted by
Sujith S
Description
The
FeeManagerallows admins to configure negative maker fees (rebates) viaupdateDefaultMakerFee(),updateFeeTier(),updateMarketFeeTier(), andupdateRelayerFeeTier()independently of the taker fee. However, the matching engine inLibMatchingEngineenforces a runtime invariant thatmakerFeeBps + takerFeeBps >= 0, reverting withNegativeNetFeeif violated.Since maker and taker fees are configured through separate admin calls with no cross-validation, an admin can set a maker rebate (e.g., -100 bps) while the taker fee remains at 0. This configuration is accepted without error but silently breaks all order matching for the affected protocol, market, or account. The failure only surfaces at match time when NegativeNetFee reverts, with no indication during configuration that the fee combination is invalid.
Recommendation
Add a validation check in the fee configuration functions that reverts when a negative maker fee is set without a sufficiently large taker fee counterpart. Alternatively, document this constraint prominently so admins are aware that maker rebates require a compensating taker fee to avoid breaking order matching.
Deviation check uses average denominator, understating price divergence
Description
The
_checkDeviation()inRisexOraclefunction calculates the percentage deviation between two Oracle prices using their average as the denominatorThis formula (known as "relative percent difference") systematically understates the deviation compared to standard percentage change. When one price is double the other (100% deviation), the formula reports only 66.67%:
price1 = 100, price2 = 200 diff = 100 average = 150deviation = 100 / 150 = 66.67% (actual: 100%)Recommendation
Use
min(price1, price2)as the denominator to match the intuitive meaning of "percentage deviation":uint256 minPrice = price1 < price2 ? price1 : price2; require(minPrice > 0, LibErrors.InvalidPrice()); uint256 deviationBps = (diff * LibConstant.BPS_DENOMINATOR) / minPrice;This ensures that a 5% threshold means exactly 5% maximum divergence from the lower price.
Shared fallback source for index and external mark reduces median fault tolerance
Description
The mark price is computed as the median of three sources for manipulation resistance:
function getMarkPrice(uint16 marketId) external view returns (uint256) { uint256 indexPrice = this.getIndexPrice(marketId); // ← fallback from s_marketOracleConfig[marketId] uint256 externalPrice = _getExternalMarkPrice(marketId); // ← fallback from SAME s_marketOracleConfig[marketId] .... }Both
getIndexPrice()and_getExternalMarkPrice()resolve their fallback from the same storage:s_marketOracleConfig[marketId].fallbackSource. There is only one fallback source per market, shared across both price types.When the primary Stork oracle goes down, the fallback becomes the sole source for both index and external mark prices.
The three median inputs degrade:
- P1 = indexPrice + clamp(EMA_premium) - anchored to fallback's index price
- P2 = impact price - falls back to indexPrice on thin/empty books
- P3 = external mark price - directly from fallback's mark price
If the fallback oracle is compromised, the attacker can control all three median inputs from a single source. The median's 2-of-3 fault tolerance collapses to 0-of-1.
This is especially dangerous on low-liquidity markets where the order book is thin, where the impact price silently degrades to indexPrice, making P2 = P1 ≈ corrupt the index. All three inputs converge to the attacker's price.
Recommendation
Consider using independent fallback sources for index and external mark price.
Unbounded gas consumption in pumpForUser() can cause OOG revert
Description
The function
pumpForUser()inPendingSettlementCleaneris designed to settle all pending deferred fills for a specific user. It identifies the user's open orders, determines their tick levels and seqIds, then calls_pumpDirtyLevel()for each level with unbounded limits:(uint32 levelOrders, uint32 levelSegments) = _pumpDirtyLevel({ protocol: protocol, marketId: marketId, side: m_sides[i], tick: m_ticks[i], maxOrders: type(uint32).max, // ← no cap maxSegments: type(uint32).max, // ← no cap stopAfterSeqId: m_cutoffSeqIds[i] });The settlement pump processes orders FIFO; it must settle every order from the queue head up to the user's seqId. If the user's order sits deep in the queue (high seqId) with many other users' orders and fill segments ahead of it, the function must process ALL of them sequentially to reach the user's order. Each order settlement involves multiple SSTOREs (tree updates, segment cursor advancement, tombstoning, hook dispatch), making gas consumption linear in the number of preceding orders.
Recommendation
Add maxOrders and maxSegments parameters to
pumpForUser(), similar to howpumpDirtyLevels()already bounds its work. This allows callers to incrementally settle across multiple transactions. Theprogress.doneflag already supports this pattern as it returns false when pending deferred fills remain.Emergency pending withdrawal discards original recipient address
State
- Acknowledged
Severity
- Severity: Low
Submitted by
Sujith S
Description
The function
withdraw(account, token, amount)supports withdrawing collateral to a different account address (e.g., a cold wallet). The sender (msg.sender) is the collateral owner, and the account is the intended token recipient.In the normal path, tokens are correctly sent to account:
IERC20(tokenAddr).safeTransfer(account, amount);However, when an oracle deviation auto-triggers emergency mode during the withdrawal,
_executeWithdraw()queues the pending withdrawal under the sender:_queuePendingWithdrawal(sender, token, amount);The PendingWithdrawal struct stores only the amount and requestedAt and has no recipient field. When
releasePendingWithdrawal(sender, token)is later called, the tokens are sent to the sender instead of the original account recipient.Recommendation
Add a recipient field to the PendingWithdrawal struct:
struct PendingWithdrawal { uint128 amount; uint64 requestedAt; address recipient; // ← store original destination}And pass the account instead of the sender to
_queuePendingWithdrawal(), or store the recipient separately and use it during releaseRise Labs
it is expected by design that an emergency event happens, released tokens will be transferred back to original owner, which can be sent to any recipient.
Permissioned keeper can bias funding rate via selective snapshot timing
Description
The
snapshotPremiumIndex()function inFundingRate.solis gated byonlyAllowed(msg.sender), meaning only permissioned keepers can trigger premium index snapshots. The funding rate TWAP is computed as a simple arithmetic average (pendingSum / pendingCount), not a time-weighted average.A malicious or compromised keeper could selectively skip snapshots when the premium index is unfavorable to their position, reducing the sample count during those periods. Since the average weights all samples equally regardless of time spacing, omitting samples during unfavorable windows biases the resulting funding rate.
The 5-second SNAPSHOT_PREMIUM_INDEX_INTERVAL prevents flooding (callers cannot exceed one snapshot per interval), and the ±4% funding clamp bounds per-settlement impact, but systematic bias across many settlements could extract value from one side of the market.
This is a standard centralization risk inherent to any keeper-operated system where a compromised keeper has broader griefing capabilities beyond funding manipulation.
Recommendation
Consider using a time-weighted average (weight each sample by elapsed time since prior snapshot) so that irregular sampling does not bias the result. Alternatively, allow settlement to be permissionlessly triggered to reduce single-keeper trust.
Liquidation silently no-ops when order book lacks depth
Description
The function
liquidate()inLibPerpsLiquidation, when_placeLiquidationOrder()returnsWideOrderId(0)due to insufficient order book depth, the function returns silently without reverting or emitting an event.This means a
liquidate()call that cancels orders but fails to reduce the position completes successfully, with no on-chain signal to distinguish it from a tx that simply had no work to do. Off-chain monitoring and keeper systems may misinterpret the successful transaction as a completed liquidation.Recommendation
Revert when
_placeLiquidationOrder()returns zero, so callers and monitoring systems get an unambiguous failure signal.Inaccurate function call can DoS if an upgrade takes place
Description
In the
RISExUniversalRouterBasecontract whenplaceOrderis called via_forwardCallWithSender, it usesIPerpsManagerinterface regardless of the manager being spot of perps.IPerpsManagerandISpotManagerinterfaces overlap in most functions, but not all. Also, if any update will be done in the future in any of the interfaces,_forwardCallWithSendermight forward the call with the wrong function selector.if (m_decoded.flags & LibRouterCodecV1.FLAG_PERMIT_ERC1271 == LibRouterCodecV1.FLAG_PERMIT_SINGLE) { m_decoded.params = _normalizePermitSinglePlaceParams(m_decoded.params); permitSingleNotional = _computeNotional(manager, m_decoded.params); } >> bytes memory returnData = _forwardCallWithSender(abi.encodeCall(IPerpsManager.placeOrder, m_decoded.params), account, manager); orderId = abi.decode(returnData, (WideOrderId)); // Post-consume: budget is only charged when the FOK order succeeds. // If the order reverts (FOK fail), the entire tx reverts and no budget is consumed. if (permitSingleNotional != 0) { _consumePermitSingle(account, msg.sender, permission, permitSingleNotional); }Recommendation
Correctly call the
placeOrderfunction from the correct Interface depending on the permission type:+ if (permission == ISignerManager.Permission.Perps) { bytes memory returnData = _forwardCallWithSender(abi.encodeCall(IPerpsManager.placeOrder, m_decoded.params), account, manager); orderId = abi.decode(returnData, (WideOrderId));+}else{+ bytes memory returnData = _forwardCallWithSender(abi.encodeCall(ISpotManager.placeOrder, m_decoded.params), account, manager); orderId = abi.decode(returnData, (WideOrderId));+ }PerpsManager is paused after re-entrancy enabled external calls
Severity
- Severity: Low
Submitted by
0xWeiss
Description
Here,
PerpsManager.pausemakes external calls toordersManager.pause()andcollateralManager.pause()before updating its own_pausedflag.function pause() external override restricted { _registryStorage().ordersManager.pause(); _registryStorage().collateralManager.pause(); _pause(); }Because collateral and order manager are registry-controlled addresses, if miss-configured on initialization or the admin gets compromised they can execute arbitrary code and reenter
PerpsManagerbecause it would still be unpaused.During that reentrancy window, any functions that have a whenNotPaused modifier remain callable, while they shouldn't.
The correct behavior is for the local pause state to become effective before any external call can occur.
Recommendation
Make sure the contract is paused before any external calls can happen:
function pause() external override restricted {+ _pause(); _registryStorage().ordersManager.pause(); _registryStorage().collateralManager.pause();- _pause(); }Rise Labs
Function clearEmergency() leaves stale metadata in storage
Description
Function
clearEmergency()inEmergencyWithdrawGuardUpgradeableonly sets$.state.isActive = false. The remaining fields, triggeredAt, triggeredBy, reason, triggeringAmountUSD, and triggeringToken retain their values from the previous emergency.This means
getEmergencyState()returns a struct where isActive == false, but all other fields are populated with data from a resolved emergency, which is misleading to off-chain consumers.Recommendation
Consider
delete $.stateinclearEmergency().Single-Oracle fallback bypasses cross-source deviation check without degraded-state handling
Severity
- Severity: Low
Submitted by
Sujith S
Description
The
_getTokenPriceWithFallback()function performs a cross-source deviation check only when both the primary and fallback oracles return valid prices. When one source is unavailable, the surviving oracle's price is accepted without cross-source validation:if (primarySuccess && fallbackSuccess) { _checkDeviation(primaryPrice, fallbackPrice, threshold); // ← only here price = primaryPrice; } else if (fallbackSuccess) { price = fallbackPrice; // no deviation check } else if (primarySuccess) { price = primaryPrice; // no deviation check}When a fallback oracle is configured, the admin has explicitly opted into dual-source validation as a safeguard against price manipulation or oracle malfunction. If one source goes down, this safeguard is silently disabled, and the system continues processing withdrawals at full speed using an unverified price with no operational signal that it is operating in a degraded state.
In
CollateralManager._toUSDWithHealthCheck, the oracle deviation errors (PriceDeviationTooHigh, StablecoinDepegDetected) trigger emergency mode and queue withdrawals as pending. However, the single-source degraded path produces no error at all, returns a price as normal, and the withdrawal executes immediately against a potentially unverified price.Recommendation
When a fallback source is configured but only one oracle is available, signal the degraded state to
CollateralManagerso that withdrawals are queued as pending rather than executed immediately, consistent with how the system already handles oracle deviation errors. This should not apply when no fallback is configured (address(0)), as the admin has accepted single-source risk by design.Wrong size of storage gaps
State
- Acknowledged
Severity
- Severity: Low
Submitted by
0xWeiss
Description
As a proper practice, storage gaps together with contract state variables should sum up storage slots to 50 and should be declared after all variables to avoid any issues on upgrades:
contract RISExOracle is IRISExOracle, AllowlistManagedUpgradeable, PartialPauseManagedUpgradeable { using LibBlockEMA for LibBlockEMA.EmaState; >> uint256[50] private __gap; IRISExStork private s_risexStork; // 20 bytes uint16 private s_deviationThresholdBps; // 2 bytes — packed with s_risexStork in 1 slot mapping(uint16 marketId => MarketOracleConfig) private s_marketOracleConfig; mapping(address token => TokenOracleConfig) private s_tokenOracle; mapping(uint16 marketId => MarketMarkOracleState) private s_marketMarkOracle; IOrdersManager private s_ordersManager; IPerpsMarketConfig private s_perpsEngine; /// @custom:oz-upgrades-unsafe-allow constructor constructor() { _disableInitializers(); }Therefore, if you calculate how storage slots are allocated in the
RISExOraclecontracts:s_risexStork + s_deviationThresholdBps -> 0 s_marketOracleConfig -> 1 s_tokenOracle -> 2 s_marketMarkOracle -> 3 s_ordersManager -> 4 s_perpsEngine ->5It sums up to 6 slots occupied, which therefore means that gap array should be of length 44.
Recommendation
Grep the entire repo for contracts that need storage gaps and calculate the correct array size. Additionally, move the gap array at the end.
- uint256[50] private __gap; IRISExStork private s_risexStork; // 20 bytes uint16 private s_deviationThresholdBps; // 2 bytes — packed with s_risexStork in 1 slot mapping(uint16 marketId => MarketOracleConfig) private s_marketOracleConfig; mapping(address token => TokenOracleConfig) private s_tokenOracle; mapping(uint16 marketId => MarketMarkOracleState) private s_marketMarkOracle; IOrdersManager private s_ordersManager; IPerpsMarketConfig private s_perpsEngine; + uint256[44] private __gap;Rise Labs
Acknowledged. We plan to migrate this contract's storage to ERC-7201 namespaced storage, which eliminates the need for gap arrays entirely.
Informational32 findings
TokenManager lacks public getter for AccountRegistry
Description
TokenManager stores an
IAccountRegistryreference in itsTokenManagerRegistryStoragestruct (set during initialization) and exposes an internal accessor_accountRegistry()inTokenManagerBase.However, unlike OrdersManager, which exposes
getAccountRegistry()publicly via OrderBookViews, TokenManager provides no external getter for the stored accountRegistry.Recommendation
Consider adding a public
getAccountRegistry()getter to TokenManager for consistency.Function deposit() in TokenManager does not revert when actualReceived Is zero
Description
The
deposit()function in TokenManager validates that the caller-supplied amount is non-zero, but after thesafeTransferFrom, it computes actualReceived via a before/after balance difference to accommodate fee-on-transfer tokens.This actualReceived value is never validated and can be zero. For instance, with a fee-on-transfer token whose transfer fee equals or exceeds the sent amount.
When actualReceived == 0, the function still:
- Registers the account via getOrRegister, permanently consuming a uint32 userId slot.
- Credits += 0 to the balance, a no-op write.
- Emits a Deposited event with a zero amount, which can mislead off-chain indexers and front-end displays.
This allows anyone to force-register arbitrary allowlisted addresses as userIds without any actual deposit, polluting the account registry and emitting deposit events.
Recommendation
Add a zero-amount check on actualReceived immediately after computing it:
uint256 actualReceived = IERC20(tokenAddr).balanceOf(address(this)) - balanceBefore;+require(actualReceived != 0, LibErrors.ZeroAmount());This ensures the defensive intent of the existing amount != 0 check extends to the actual transferred value.
Function updateBalanceByUserId() in TokenManager allows negative balances without explicit validation
Description
The
updateBalanceByUserId()function inTokenManagerapplies the caller-supplied delta to the user's balance without checking whether the resulting newBalance is negative:int256 oldBalance = $.balance[token][userId];int256 newBalance = oldBalance + delta;$.balance[token][userId] = newBalance; // no non-negative checkAll callers go through LibSpotHooks (_settleBalances, onTakeLevel, onSettleMakerChunk), where negative deltas are applied for the sell side of a match (e.g., -matchedSizeInt for base tokens, -matchedQuoteInt - takerFee for quote tokens).
While upstream order placement in LibSpotOrders validates sufficient available balance before locking and performs a post-match solvency check for buy orders, the settlement function itself enforces no invariant. If a future caller is introduced or upstream checks are refactored, negative balances could be created silently and go undetected. Though the current system is not exploitable, it is good to have the checks in here for future safety.
Recommendation
Add a non-negative balance assertion in updateBalanceByUserId for the token being debited, or document the function's lack of solvency checks with a @dev note explaining the reliance on upstream validation.
Function setSupportedToken() emit SupportedTokenUpdated event when no state change occurs
Description
The function
setSupportedToken()inTokenManager.solemitsSupportedTokenUpdatedunconditionally after the conditional block, including when the requested state matches the current state (e.g., adding an already-supported token or removing an already-unsupported token). In these cases, neither the if nor the else if branch executes, no storage is modified, yet the event is still emitted.Recommendation
Move the event emission inside each branch, or add an early return when no state change occurs.
Three of seven selector checks in _requireValidPauseSelector() are dead validations
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Sujith S
Description
The
_requireValidPauseSelector()function blocks 7 selectors from being stored in the per-function pause mapping s_pausedFn. Only four serve a purpose: pause.selector, pauseFunction.selector, unpauseFunction.selector, and unpause.selector as they protect the global pause/unpause mechanism from self-lockout.The remaining 3 protect selectors whose functions never route through the overridden
_requireNotPaused()function, making it a dead validation.Recommendation
Remove the 3 dead checks to reduce bytecode size and avoid implying these functions are pausable.
Rise Labs
The extra checks are intentional defense-in-depth, they prevent these selectors from ever being added to the pause mapping regardless of whether the functions currently route through _requireNotPaused(). Adding a pausedFunction or supportsInterface entry to s_pausedFn would be semantically nonsensical, and the blocklist guards against misconfiguration. The gas cost is negligible since this only runs during admin configuration calls.
Outdated natspec references PROTOCOL_ROLE in AccountRegistry and multiple interfaces
Description
The
AccountRegistrycontract-level NatSpec states "Only addresses holding PROTOCOL_ROLE may call getOrRegister", and similarPROTOCOL_ROLEreferences appear acrossIAccountRegistry, IOrdersManager, andIExpiryOrderCleaner`.However,
PROTOCOL_ROLEis a legacy alias (LibConstant.sol line 165: PROTOCOL_ROLE = PAUSER_ROLE), and the actual access control is delegated entirely to the externalAccessManagervia the restricted modifier, and no specific role is enforced at the contract level. ,Recommendation
Update all NatSpec references from
PROTOCOL_ROLEto a generic description reflecting the actual mechanism:/// @dev Access restricted via AccessManager (`restricted` modifier).Unsafe uint256 to uint96 downcast in consumeAllowance() function
Description
The function
consumeAllowance()andconsumeAllowanceOnBehalf()inPermitSingle.solboth downcast the uint256 amount parameter to uint96 without an explicit safe cast.No current code path can produce an amount that both passes the budget check and truncates on downcast. However, it's a good practice to safely cast down.
Recommendation
Replace the unchecked cast with an explicit safe cast for clarity and defense-in-depth:
$.budget -= SafeCastLib.toUint96(amount);Functions updateTokenBalanceByUserId() and updateTokenBalance() has inconsistent modifier implementation
Severity
- Severity: Informational
Submitted by
Sujith S
Description
CollateralManager exposes two balance update functions with inconsistent modifier coverage:
// Has nonReentrant + whenNotPaused + ensureNonZeroAddressfunction updateTokenBalance( address account, Currency token, int256 delta) external nonReentrant whenNotPaused ensureNonZeroAddress(account) ensureNonZeroAddress(Currency.unwrap(token)) onlyPerpsManager // Only has onlyPerpsManagerfunction updateTokenBalanceByUserId(uint32 userId, Currency token, int256 delta) external onlyPerpsManager { ... }The function
updateTokenBalanceByUserId()lacks nonReentrant, whenNotPaused, and zero-address/zero-token validation that its address-based sibling enforces, though both functions perform the identical storage operation.In the current codebase, this has no exploitable impact. All callers are internal Perps libraries (LibPerpsHooks, LibPerpsOrders, LibPerpsLiquidation, LibPerpsRLP, LibPerpsMigration) that execute within PerpsManager entry points, which already enforce nonReentrant and whenNotPaused at the top of the call stack.
Additionally,
updateTokenBalance()(the address version with full modifiers) has zero callers; onlyupdateTokenBalanceByUserId()is used in practice, making the inconsistency purely cosmetic.Recommendation
- Add nonReentrant and whenNotPaused modifiers to
updateTokenBalanceByUserId()for defense-in-depth and consistency withupdateTokenBalance(). - Or remove the
updateTokenBalance()function since it's unused.
Function getTotalTokenBalanceInUSDByUserId() implements unbounded loop over supported tokens
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Sujith S
Description
The
getTotalTokenBalanceInUSDByUserId()function inCollateralManageriterates over the'supportedTokens' enumerable to compute a user's total collateral value in USD.Each iteration performs a storage read for the user's balance and (for non-USDC tokens) an external oracle call via
getTokenPrice(). If the number of supported collateral tokens grows unboundedly, this function could approach the block gas limit. Since this view function is called on-chain during withdrawal health checks (_toUSDWithHealthCheck()) and margin calculations, a revert would block withdrawals and liquidations for all users.Recommendation
Add a maximum supported token count check in
setIsCollateralToken()as a defensive bound.Rise Labs
Acknowledged . The DoS issue is not feasible for our current chain config (1.5B gaslimit). If in the future we are expanding supported tokens large enough, we will fix this.
Function requireMarginAfterTrade() could use the market-aware loadFundingPayment() function
Description
The
requireMarginAfterTrade()function inLibPerpsMarginChecksin the isolated margin path loads the cached funding payment using the no-argument overload:(accumulatedFundingPayment, fundingHit) = LibPerpsTransientCache.loadFundingPayment();This overload reads from FUNDING_PAYMENT_SLOT and only checks the initialized bit, and it does not validate that the cached value corresponds to the marketId being checked. A market-aware overload
loadFundingPayment(uint16 marketId)exists in LibPerpsTransientCache that validates the FUNDING_PAYMENT_META_SLOT against the requested marketId, but it is not used here.In the current codebase, this is safe: all callers (LibPerpsOrders.placeOrder, LibPerpsRLP, LibPerpsLiquidation) operate on a single market per transaction, and clearAll() flushes the transient cache at the end of each operation.
However, if a future code path introduces multi-market operations within a single transaction without an intermediate clearAll(), the no-argument loadFundingPayment() could return a stale funding payment from a previously processed market, leading to incorrect isolated margin calculations.
Recommendation
Use the market-aware overload for defensive consistency.
No-op state changes emit misleading events in RISExAccessManagerUpgradeable
Description
The functions
setWhitelistEnabled(),setBlacklistEnabled(),setWhitelisted(), andsetBlacklisted()inRISExAccessManagerUpgradeableunconditionally write to storage and emit events without first checking whether the new value differs from the current value.This has three consequences:
-
Misleading event logs: Off-chain indexers, monitoring dashboards, and compliance systems that track allowlist changes cannot distinguish real state transitions from no-ops. A WhitelistedAccount(alice, true) event could represent either a new whitelisting or a redundant call, degrading the reliability of the audit trail.
-
Wasted gas: Each call performs a storage write and event emission even when the stored value is identical, costing unnecessary gas for the onlyAuthorized caller.
-
Compliance risk: For a protocol that uses allowlists for KYC/sanctions enforcement, an inaccurate event history undermines the trustworthiness of on-chain compliance records that regulators or auditors may rely on.
Recommendation
Add an early return guard to each setter that skips the write and event when the value is unchanged.
-
Missing zero-address validation on ordersManager.getAccountRegistry() return value
Description
The function
__SpotManagerBase_init_unchained()inSpotManagerBaseand__PerpsManagerBase_init_unchained()inPerpsManagerBasestore the return value fromordersManager.getAccountRegistry()without validating that it is non-zero.If
OrdersManageris not yet initialized, this silently stores address(0). This is inconsistent with all other accountRegistry assignments in the codebase (OrdersManagerBase, CollateralManager, FeeManager, TokenManager), which validate against zero.Recommendation
Add
_requireNonZeroAddress(address($.accountRegistry))after the assignment.Missing accountRegistry() getter in SpotManagerBase
Description
SpotManagerRegistryStorage stores four dependencies:
ordersManager(),tokenManager(),feeManager(), andaccountRegistry(). However, SpotManagerBase exposes getters for the first three (getOrdersManager(),getTokenManager(),getFeeManager()) but not for accountRegistry.Recommendation
Add
getAccountRegistry()toISpotRegistryand implement it inSpotManagerBaseFunction getMarketConfig() in SpotMarketConfig returns default struct for invalid market IDs
Description
The function
getMarketConfig()performs no validation check while querying a non-existent marketId (e.g., 0, or beyond totalMarkets), and returns a zero-initialized SpotMarketConfig struct rather than reverting.This is inconsistent with
getMarketIdByPair(), which explicitly reverts withPairNotFounderror.Recommendation
Consider adding a bounds check for consistency, or document the behavior as intentional.
Function swapTransferOut() does not verify locked balance
Description
Function
swapTransferOut()inTokenManagerchecksnewBalance >= 0but does not subtract locked balance, unlikewithdraw()function which checksbalance - locked >= amount.On its own, this is not exploitable; all three call context in LibSpotSwap only transfer deltas produced by the swap itself (output fills, unspent input refunds, intermediate sweeps), and
executeHop()already computesavailable = balance - lockedbefore consuming tokens.However, any future caller of
swapTransferOut()that passes a non-delta amount could bypass locked balance protections.Recommendation
Add the locked balance check for consistency with
withdraw()function:uint256 locked = _registryStorage().spotManager.getTotalLockedBalance(account, Currency.unwrap(token)); require(newBalance >= locked.toInt256(), InsufficientBalance(account, Currency.unwrap(token), currentBalance, amount));Missing natSpec documentation on _getBuilderCodeStorage()
Description
The internal function
_getBuilderCodeStorage()inFeeManagerBase.sollacks NatSpec documentation, while the adjacent functions_registryStorage(),_getFeeStorage(), and_accountRegistry()all have @notice and @return annotations.Recommendation
Add natSpec to
_getBuilderCodeStorage()consistent with other functions inFeeManagerBaseMissing getters for multiple internal storage variables in FeeManager
Description
The
FeeManagercontract lacks external getters for several internal storage variables. The following fields are inaccessible to off-chain consumers without raw storage slot inspection:- FeeManagerRegistryStorage.accountRegistry
- BuilderCodeStorage.nextBuilderId
- BuilderCodeStorage.protocolByFeeVault
- BuilderCodeStorage.vaultConfigByProtocol.outstandingFeesWad
Recommendation
Consider adding getter functions for the above-mentioned variables in
FeeManagerRelayer fee tier lookup via tx.origin incompatible with ERC-4337 account abstraction
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Sujith S
Description
The functions
_getMakerFeeBpsByUserId()and_getTakerFeeBpsByUserId()usetx.originto resolve relayer-specific fee tiers:address relayer = tx.origin;if ($.relayerFeeTiers[relayer].maker.isSet) { return $.relayerFeeTiers[relayer].maker.feeBps;}Under ERC-4337, user operations are submitted to a bundler, which calls the EntryPoint contract, making
tx.originthe bundler's EOA rather than the relayer that originally submitted the user operation. If the protocol ever supports AA-based order submission, relayer fee tiers configured for specific relayer addresses would silently fail to match, falling through to market or default fees instead. There is no risk of exploitation, as relayer fee tiers are set by admins via restricted functions. The concern is that relayer-specific pricing would not apply as intended.Recommendation
If ERC-4337 support is planned, consider accepting the relayer address as an explicit parameter in the fee lookup path rather than relying on tx.origin. Otherwise, document that relayer fee tiers require direct EOA submission and are incompatible with AA-routed transactions.
Rise Labs
This is expected by design and we won't support ERC4337.
Missing event in deductFees() function
Description
The function
deductFees()inFeeManagerreduces totalFeesCollected for a given protocol and token but emits no event. Its counterpartcollectFees()emits CollectedFees on every fee pool increase, creating an asymmetry in observability. Off-chain indexers and monitoring systems that track protocol fee balances via events will have an incomplete picture, as they can detect fee inflows but not outflows via deductFees.Recommendation
Emit a DeductedFees(address protocol, Currency token, uint256 amount) event (or similarly named) after the state mutation, mirroring the pattern used by collectFees. Add the event definition to IFeeManager.
Unused SafeCastLib import in RISExStork
Description
The contract
RISExStork.soldeclaresusing SafeCastLib for *;and imports SafeCastLib, but no function in the contract calls any SafeCastLib method.Recommendation
Remove the import and using declaration:
- import { SafeCastLib } from "solady/src/utils/SafeCastLib.sol";... - using SafeCastLib for *;Missing getTokenPriceId() getter in RISExStork
Description
The contract
RISExStorkhas functionsgetMarkPriceId(uint16)andgetIndexPriceId(uint16)for market price feed lookups, but has no equivalent getter for s_tokenPriceIds. The mapping is private, so the configured Stork feed ID for a given token is unreadable externally.Recommendation
Add a matching getter:
function getTokenPriceId(address token) external view returns (bytes32) { return s_tokenPriceIds[token]; }No independent staleness check on Stork oracle prices
Description
The function
_getPrice()inRISExStorkcallss_stork.getTemporalNumericValueV1(priceId)and validates only thatquantizedValue > 0. The returned timestampNs field is completely ignored; the protocol performs no independent age check.Staleness protection relies entirely on Stork's internal threshold. If the Stork contract is upgraded, its staleness threshold is reconfigured, or the check is disabled, arbitrarily stale prices would flow into mark price computation, collateral valuation, and liquidation decisions without any protocol-level safeguard.
Recommendation
Consider adding a configurable maximum age parameter and validating timestampNs against block.timestamp in
_getPrice()as a defense-in-depth measure independent of Stork's internal checks.Stablecoin peg deviation upper bound allows up to 100% depeg
Description
The
setStablecoinPeg()function validates that maxPegDeviationBPS is at most LibConstant.BPS_DENOMINATOR (10000 = 100%). This means an admin could configure a stablecoin with a 100% deviation tolerance. A token pegged at $1.00 would pass validation at $2.00 or $0.00, completely defeating the depeg protection.Recommendation
Introduce a maximum upper bound constant with a reasonable peg deviation value (say 5%) and enforce it.
Missing input validation on oracle deviation threshold configuration
Description
Both
setDeviationThreshold()andsetMarketDeviationThreshold()accept any uint16 value as a deviation threshold with no bounds validation. This contrasts with other Oracle configuration functions that do validate inputs.Recommendation
Add bounds validation consistent with the pattern used elsewhere:
uint16 public constant MAX_DEVIATION_THRESHOLD_BPS = 1_000; // 10% function setDeviationThreshold(uint16 thresholdBps) external restricted whenNotPaused { require(thresholdBps > 0 && thresholdBps <= MAX_DEVIATION_THRESHOLD_BPS, InvalidParameter()); ....} function setMarketDeviationThreshold(uint16 marketId, uint16 thresholdBps) external restricted whenNotPaused { require(thresholdBps <= MAX_DEVIATION_THRESHOLD_BPS, InvalidParameter()); ....}Cantina
Both functions now have bounds validation. The recommendation also suggested thresholdBps > 0 for setDeviationThreshold but the fix allows 0 - an intentional design choice.
Missing validation on minUpdateInterval and timeConstantSeconds in configureMarkOracle function
Description
The
configureMarkOracle()function validatesmaxPremiumBpsbounds but skips validation of' minUpdateInterval' (accepts any value, including 0) and allows' timeConstantSeconds' as low as 1.While
minUpdateInterval = 0disables the EMA rate limiter inLibMarkOracle.sol, and timeConstantSeconds = 1 causesexp(-dt/1)to erase 95% of EMA history within 3 seconds.Given that
syncMarkPriceEMA()is permissionless (RISExOracle.sol), a brief order-book manipulation can overwrite the EMA baseline almost instantly. This contrasts withconfigureTokenEMA(), which validates both alpha and minBlocks.Recommendation
Consider adding reasonable guardrails while configuring the above two variables in the
configuraMarkOraclefunction.Cantina
The issue is fixed partially. timeConstantSeconds guarded (min 10s), minUpdateInterval unchanged.
Impact price derived from resting orders is susceptible to spoofing
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Sujith S
Description
Impact price is computed by walking resting limit orders in the book up to a target notional (
IMPACT_NOTIONAL_BASE_USDC * maxLeverage). On CEXs, this same design is used, but spoofing is deterred by KYC, surveillance, and legal consequences.On a permissionless on-chain perps exchange, these deterrents don't exist. An attacker can:
- Place large limit orders near the market to skew bid/ask depth
- Wait for a premium index snapshot (FundingRate.sol) or call syncMarkPriceEMA (RISExOracle.sol, permissionless)
- Cancel orders immediately after
The primary attack surface is the funding rate manipulation. The current system includes multiple mitigations, limiting the severity of the issue.
Recommendation
Consider one or more of the following defenses to harden the system:
- Minimum order duration: orders must rest for N blocks before counting toward impact price, preventing same-block place-and-cancel
- TWAP-based impact price: average impact price over a time window rather than a point-in-time snapshot
- Execution-weighted depth: weight price levels by recent fill volume, not just resting size
Rise Labs
Acknowledged this issue, however fixing this requires a big refactor. We will continue to observe behavior on production to see if it is worth to do this. For the mean time, we consider this as low priority.
ADL zero price staleness for cross-margin multi-market positions
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Sujith S
Description
The
_processADLCounterparties()function inLibPerpsADLcomputes the zero price once and reuses it for all counterparty trades in_executeADLLoop().Each placeOtcOrder call settles the underwater account via LibPerpsSettlement.settle(), updating its position size, quoteAmount, and funding state, but the zero price is never recalculated.
For isolated-margin and single-market cross-margin positions, this is correct: zero-fee OTC trades at zero price reduce marginBalance and mm proportionally, so the zero price remains constant.
For cross-margin multi-market positions, the formula
adj = M_i * crossMarginBalance / totalMMviolates proportionality because totalMM includes other markets' unchanged MM balances. Impact is considered negligible: counterparties have post-trade margin checks, drift is second-order, and ADL only fires on deeply underwater accounts.Recommendation
Recalculate the cross-margin zero price after each counterparty fills in
_executeADLLoop(), matching the pattern in_processCrossTakeover().Rise Labs
ADL processes a single market at a time, but the zero price does drift slightly across counterparty fills because crossMarginBalance and totalMM both change after each OTC trade. However, the drift is second-order: both numerator and denominator decrease proportionally, and counterparties are protected by post-trade margin checks. The gas cost of recalculating per iteration (re-reading cross balance + iterating the portfolio bitmap) outweighs the marginal precision gain for an already deeply underwater account.
Redundant assignment of the pathLength variable
Description
In the
swapExactIntheparams.path.lengthargument is assigned to a variable mutliple times across hthe function while it should only be assigned once:function swapExactIn( SpotMarketStorage storage s_spotMarket, SpotAccountStorage storage s_account, SpotManagerRegistryStorage storage s_registry, ISpotManager.SwapExactInParams calldata params, address account ) internal returns (uint256 amountOut) {>> uint256 pathLength = params.path.length;Recommendation
Assign the variable at the start of the swaps and update all of the instance below:
function swapExactIn( SpotMarketStorage storage s_spotMarket, SpotAccountStorage storage s_account, SpotManagerRegistryStorage storage s_registry, ISpotManager.SwapExactInParams calldata params, address account ) internal returns (uint256 amountOut) {+ uint256 pathLength = params.path.length;- require(params.path.length >= 2, ISpotManager.SwapPathTooShort());+ require(pathLength >= 2, ISpotManager.SwapPathTooShort());- require( params.path.length - 1 <= MAX_SWAP_HOPS, ISpotManager.SwapPathTooLong(params.path.length - 1, MAX_SWAP_HOPS));+ require( pathLength - 1 <= MAX_SWAP_HOPS, ISpotManager.SwapPathTooLong(pathLength - 1, MAX_SWAP_HOPS)); require(params.deadline > block.timestamp, ISpotManager.SwapDeadlineExpired(params.deadline, block.timestamp)); require(params.amountIn != 0, LibErrors.ZeroAmount()); require(params.to != address(0), LibErrors.ZeroAddress()); address inputToken = params.path[0];- address outputToken = params.path[params.path.length - 1];+ address outputToken = params.path[pathLength - 1]; // Prevent round-trip swaps (same input and output token) // Balance delta calculation is ambiguous when input == output require(inputToken != outputToken, ISpotManager.SwapSameToken(inputToken)); // Prevent duplicate tokens in path — cyclic routes corrupt refund/sweep accounting // because _refundUnspentInput assumes the input token's balance delta is from hop 0 only. // O(n^2) is fine: path length is bounded by MAX_SWAP_HOPS + 1 = 6 (max 15 comparisons). {- uint256 pathLength = params.path.length; for (uint256 i; i < pathLength; ++i) { for (uint256 j = i + 1; j < pathLength; ++j) { require(params.path[i] != params.path[j], ISpotManager.SwapDuplicateToken(params.path[i])); } } } // Pull ERC20 from user into TokenManager; measure actual received for fee-on-transfer tokens uint256 actualReceived; { address tokenMgrAddr = address(s_registry.tokenManager); uint256 balBefore = IERC20(inputToken).balanceOf(tokenMgrAddr); IERC20(inputToken).safeTransferFrom(account, tokenMgrAddr, params.amountIn); actualReceived = IERC20(inputToken).balanceOf(tokenMgrAddr) - balBefore; } s_registry.tokenManager.updateBalance(account, Currency.wrap(inputToken), actualReceived.toInt256()); // Snapshot balances before swap (input, output, and intermediates for residual sweep) int256 inputBalanceBefore = s_registry.tokenManager.getBalance(account, Currency.wrap(inputToken)); int256 outputBalanceBefore = s_registry.tokenManager.getBalance(account, Currency.wrap(outputToken)); int256[] memory intermediateBalsBefore = _snapshotIntermediateBalances(s_registry.tokenManager, params, account); // Execute each hop with budget-capped input { uint256 hopBudget = actualReceived;- uint256 pathLength = params.path.length; for (uint256 i; i < pathLength - 1; ++i) { hopBudget = _executeHopAndMeasure(s_spotMarket, s_account, s_registry, params, account, i, hopBudget); } } }Test suite skewed toward happy-path coverage, lacking adversarial and boundary testing
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Sujith S
Description
Automated analysis of the test suite (~1,797 tests across 85 files) found that approximately 53% of tests validate the happy-path behavior, while only 1% (~17 tests) target security-specific scenarios. Several critical adversarial test categories are entirely absent.
While the library-level unit tests and authorization/signature tests are thorough, the integration and system-level tests primarily confirm that the protocol functions correctly under expected conditions rather than proving it is resilient under adversarial ones.
Recommendation
Consider expanding the test suite and rigorously test adversarial and boundary conditions on top of happy paths to ensure the protocol operates as expected under all possible scenarios.
Rise Labs
Acknowledged. We planned to improve the test suite in future.
Parameter amountUSD in _triggerEmergency() is always zero
Description
Both call context pass 0 for
amountUSD:- Manual trigger: _triggerEmergency(address(0), 0, reason, msg.sender)
- Auto-trigger in CollateralManager._toUSDWithHealthCheck: _triggerEmergency(tokenAddr, 0, "ORACLE_DEVIATION", address(this))
The uint128
triggeringAmountUSDfield occupies a storage slot, is emitted inEmergencyTriggered, and is readable viagetEmergencyState(), but it is never populated with a meaningful value. Off-chain monitoring systems parsing the event would seeamountUSD = 0and may incorrectly conclude the triggering exposure was negligible.Recommendation
Either compute and pass the actual USD amount at the trigger site, or remove the field from the struct and event to avoid dead storage and misleading emissions.
Unsafe uint256 to uint96 downcast in claimBuilderFees()
Description
The function
claimBuilderFees()in FeeManager.sol downcasts theuint256amount parameter touint96without an explicit safe cast.$.builderFeesByProtocol[protocol][builderId][token] = 0; // forge-lint: disable-next-line(unsafe-typecast) $.vaultConfigByProtocol[protocol].outstandingFeesWad -= uint96(accumulated); IFeeWithdrawal(tokenManager).transferTradingFees(token, amount, feeRecipient); emit ClaimedBuilderFees(builderId, tokenAddr, amount, feeRecipient);The possible amount to be accumulated should never surpass
uint96, however, it's a good practice to safely cast down.Recommendation
Replace the unchecked cast with an explicit safe cast:
$.builderFeesByProtocol[protocol][builderId][token] = 0; // forge-lint: disable-next-line(unsafe-typecast)- $.vaultConfigByProtocol[protocol].outstandingFeesWad -= uint96(accumulated);+ $.vaultConfigByProtocol[protocol].outstandingFeesWad -= SafeCastLib.toUint96(accumulated); IFeeWithdrawal(tokenManager).transferTradingFees(token, amount, feeRecipient); emit ClaimedBuilderFees(builderId, tokenAddr, amount, feeRecipient);A batchUpdateFeeTiers function would reduce operational complexity
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
0xWeiss
Description
Each function that updates a fee tier
updateFeeTier,updateRelayerFeeTier,updateMarketFeeTierhandles a single variable at a time.Example:
function updateDefaultTakerFee( address protocol, int16 newDefaultTakerFeeBps ) external restricted whenNotPaused { _requireValidTakerFeeBps(newDefaultTakerFeeBps); FeeStorage storage $ = _getFeeStorage(protocol); int16 oldDefaultTakerFeeBps = $.defaultTakerFeeBps; $.defaultTakerFeeBps = newDefaultTakerFeeBps; emit UpdateDefaultTakerFee(protocol, oldDefaultTakerFeeBps, newDefaultTakerFeeBps); }A function like
batchUpdateFeeTierswould be beneficial at reducing operational complexity and update all the tier of fees in only one call. Lot of validations are shared across functions too, so it wouldn't be very complicated to apply.Recommendation
Create a
batchUpdateFeeTiersvariable to batch update multiple fee tiers at the same time.Rise Labs
Acknowledged. We will improve this in the future. As for now, it is a rarely use operation and low priority.