Ostium audit

A private security review of Ostium (RWA perpetuals DEX), conducted with Three Sigma and completed on 19th February 2024 over 33 days. 0xSimao disclosed 57 findings: 8 high severity, 11 medium, 13 low and 25 informational.

What Ostium is

Ostium is a perpetuals DEX purpose-built for real-world assets: a stablecoin-settled trading engine with a dynamic fee structure and an in-house oracle lets traders take leveraged synthetic positions on commodities, FX and other traditional markets, fully on-chain.

Scope

The Three Sigma review covered the trading storage and lifecycle: position opening and closing, funding-fee accrual on both sides of a pair, liquidation timeouts, oracle fee handling, and the pair-update and blacklist administration paths.

High severity findings

• In OstiumTradingStorage, firstEmptyTradeIndex() and firstEmptyOpenLimitIndex() overwrite index 0 if not found
• Liquidations can be prevented by updating the SL timeout before it expires
• Uneven getPendingAccFundingFees() leading to wrong funding rate update
• Inability to Close Positions with Significant Positive PnL
• Oracle fees should be payed upfront to protect the protocol from failed performeUpkeep() calls
• updatePair() is missing the pairOk() modifier
• USDC Blacklisting Prevents Trader Liquidation
• Casting from int256 to uint256 won't revert if the number is negative, possibly leading to issues

Medium severity findings

• utilizationThresholdP of 10_000 will make OstiumPairInfos::_getUtilizationOpeningFee() divide by 0
• OstiumVault::lockDiscount() may revert due to division by 0
• OstiumTradingCallbacks::executeAutomationOpenOrderCallback() reverts if it can not find the openLimitOrder
• OstiumTrading::executeAutomationOrder() and _getLimitOrdersToTrigger() should check isPaused for open limit orders
• Missing Pausability Check in OstiumTrading::executeAutomationOrder() for Opening Orders
• OstiumTrading::topUpCollateral() is missing pairsStored.groupMaxCollateral(pairIndex) check
• OstiumTrading::closeTradeMarket() and OstiumTrading::topUpCollateral() are missing pending trigger checks
• Error in OstiumPairsStorage::groupMaxCollateral() calculation
• OstiumPriceUpKeep::performUpKeep() does not correctly handle possible reverts
• Trader Can Set Wrong TP and SL
• Setting maxFundingFeePerBlock to a lower value than abs(lastFundingRate) will brick getPendingAccFundingFees()

Low severity findings

• OstiumTradesUpKeep Triggers Automation for Pending Orders Expected to Fail
• Limitations for Users Employing Multi-sig or Account Abstraction Wallets in Setting Delegate Address
• executeAutomationOpenOrderCallback Executes STOP Orders at a Worse Price
• reqID_pendingAutomationOrder stores the index of the trade, which could point to a different trade since the request was created
• OstiumTradingCallbacks::executeAutomationOpenOrderCallback() opens limit orders at market price instead of the limit price
• OstiumPairsStorage::getAllPairsMaxLeverage() reverts if enough pairs are created
• OstiumRegistry should disable renouncing ownership if it is never intended
• OstiumLinkUpKeep:topUp() is missing a length check for registryAddresses
• .values() may revert when calling getWatchList() due to OOG
• In OstiumLinkUpKeep, empty watchlist or keeperIds arguments are not handled
• Using transfer() instead of call() may revert
• Missing disableInitializers() call in the constructor
• Ownable2Step is recommended over Ownable

Informational findings

• Implement Storage Gap in Delegatable Contract
• Unused Trade Size Variable in OstiumTrading::executeAutomationOrder()
• Implement Custom Errors Instead of require Statements
• Inconsistent Precision of Percentage Variables
• Unnecessary Typecasting of msg.sender
• Use of Magic Numbers
• Unused and Wrong Imports
• Redundant Calculations in OstiumTrading::openTrade()
• OstiumTrading::closeTradeMarket() Makes Redundant Call to OstiumTradingStorage::getOpenTradeInfo()
• OstiumLinkUpKeep Config Not Set In initialize()
• OstiumPriceUpKeep Verifies Reports Using Native Token Instead of LINK
• OstiumTrading::updateOpenLimitOrder() Makes Unnecessary Field Updates
• OstiumTrading::canExecute() should also be checked in OstiumTradesUpKeep::checkCallback()
• Unused UPDATE_SL OstiumPriceUpKeep::OrderType
• In OstiumTradesUpKeep, _getLimitOrdersToTrigger() and _getOpenOrdersToTrigger() return tradesToTrigger with 1 extra length
• Wrong decoding of verifierResponse in OstiumPriceUpKeep::performUpkeep()
• updateGroupCollateral() should revert if the _pairIndex does not exist
• Error parameters should be more descriptive
• Faulty revert reason decoding in Delegatable
• OstiumOpenPnl::average() can be simplified by adding a state variable that tracks the cumulative sum of nextEpochValues
• OstiumOpenPnl::nextEpochValuesRequestCount stores the same information as nextEpochValues.length
• Redundant abi.decode() in OstiumTradesUpKeep::checkCallback()
• Consider using forceApprove instead of safeApprove in OstiumTraidingCallbacks
• OstiumLinkUpKeep:removeFromWatchList() complexity could be reduced
• Typos in OstiumLinkUpKeep

The report

Download report (PDF)