fix: Security Audit Fixes for Dex223Oracle.sol and OracleLibrary.sol

[rroland10]

Security Audit Report: Dex223Oracle.sol and OracleLibrary.sol

This PR contains comprehensive security audits for both oracle-related contracts in the Dex223 system:

1. Dex223Oracle.sol (dex-core) - 9 vulnerabilities found and fixed
2. OracleLibrary.sol (dex-periphery) - 8 vulnerabilities found and fixed

---

Part 1: Dex223Oracle.sol Audit

Summary

Full security audit of contracts/dex-core/Dex223Oracle.sol -- the price oracle contract used by the Dex223 Margin Module for position valuation, liquidation decisions, and collateral estimation. Nine vulnerability classes were identified and fixed.

---

Vulnerability Findings

V1 -- Mutable pricePrecisionDecimals (Medium)

Severity: Medium | Category: Access Control / Configuration Integrity

Description: pricePrecisionDecimals was declared as a regular mutable state variable (uint256 public pricePrecisionDecimals = 5). Since the Oracle contract has no access control, any external interaction pattern that upgrades or redeploys could inadvertently change this value. More critically, there is no setter function, but the variable being mutable means a storage collision in a proxy/delegatecall pattern or an unintended inheritance override could silently change it. Additionally, mutable storage costs more gas on every read.

Fix: Changed to immutable, set in constructor. This guarantees the value is embedded in the deployed bytecode and cannot be altered.

---

V2 -- Mutable factory and feeTiers (Medium)

Severity: Medium | Category: Access Control / State Integrity

Description: factory was a regular public state variable and feeTiers was a mutable dynamic array. Since there are no access controls or setter restrictions:

• A mutable factory in a proxy/delegatecall context could be pointed to a malicious factory returning attacker-controlled pool addresses, enabling price manipulation.
• Mutable feeTiers array could be altered to skip valid fee tiers or add invalid ones, causing the oracle to select suboptimal or malicious pools.
• Dynamic storage arrays also cost more gas to iterate than constants.

Fix:

• factory made immutable with a zero-address check in constructor.
• feeTiers replaced with three private constant values and a pure helper function _feeTier(uint256 idx).
• A public feeTiers(uint256 idx) view function preserves backward compatibility for external consumers.

---

V3 -- Missing Pool Address Validation in getSqrtPriceX96 (High)

Severity: High | Category: Input Validation

Description: getSqrtPriceX96(address poolAddress) accepted address(0) as a pool address without validation. In Solidity 0.7.6, calling .slot0() on address(0) (an EOA with no code) would silently return all-zeros instead of reverting. This means the function would return sqrtPriceX96 = 0, causing all subsequent price calculations to return 0. In the margin module context, this would make positions appear to have zero value, potentially triggering incorrect liquidations or, worse, allowing undercollateralized positions to avoid liquidation if the zero propagates in unexpected ways.

Fix: Added require(poolAddress != address(0), "Oracle: zero pool") at the start of both getSqrtPriceX96 and getSpotPriceTick.

---

V4 -- Uninitialized Pool Price Not Rejected (High)

Severity: High | Category: Data Validation / Economic Safety

Description: If a Uniswap V3 pool exists but has not been initialized (no initialize() call), slot0().sqrtPriceX96 returns 0. The Oracle would pass this zero value into price calculations, producing zero-valued outputs. When used in the Margin Module, this would value all non-base assets at 0, causing:

• Healthy positions to appear undercollateralized (triggering unjust liquidations).
• The getAmountOut inverse-price branch to divide by zero (revert without helpful error).

Fix: Added require(sqrtPriceX96 > 0, "Oracle: pool not initialized") after the slot0() call.

---

V5 -- Zero amountToSell Causes Division-by-Zero (Medium)

Severity: Medium | Category: Input Validation / Arithmetic Safety

Description: Both getAmountOut and getAmountOutIntrospection accepted amountToSell = 0 without validation. When sell > buy (inverse price branch), the code computes amountToSell * amountToSell / amountBought. With amountToSell = 0, amountBought also becomes 0, causing a division-by-zero revert with no helpful error message.

Fix: Added require(amountToSell > 0, "Oracle: zero amount") at the start of both functions.

---

V6 -- Intermediate Overflow in sqrtPriceX96 Squared Computation (Critical)

Severity: Critical | Category: Arithmetic Overflow

Description: The original code computed uint256(getSqrtPriceX96(_pool))**2 * sum / 2**192. Since sqrtPriceX96 is a uint160, squaring it produces a value up to 2^320, which overflows uint256 (max 2^256 - 1). In Solidity 0.7.6 (no built-in overflow checks), this silently wraps around, producing a completely incorrect price.

Fix: Introduced _safePriceCalc(uint256 sqrtPrice, uint256 amount) which splits the computation into two steps that keep all intermediates below 2^256.

---

V7 -- Division-by-Zero in Inverse-Price Branch (Medium)

Severity: Medium | Category: Arithmetic Safety

Description: When sell > buy, both getAmountOut functions compute amountToSell * amountToSell / amountBought. If amountBought evaluates to 0, this causes an unguarded division-by-zero.

Fix: Added require(amountBought > 0, "Oracle: zero price result") before the division in both functions.

---

V8 -- Missing tokenB Zero-Address Validation (Low)

Severity: Low | Category: Input Validation

Description: findPoolWithHighestLiquidity validated tokenA != address(0) but not tokenB.

Fix: Added require(tokenB != address(0), "Oracle: zero address tokenB").

---

V9 -- Missing Revert Reason Strings (Informational)

Severity: Informational | Category: Debugging / Operability

Description: All require statements in the original code lacked error message strings.

Fix: Added descriptive revert reason strings to all require statements throughout the contract.

---

Dex223Oracle.sol Additional Improvements

• Removed dead commented-out code for improved readability
• Cleaned up unused variables
• Gas optimization: replacing mutable storage array iteration with constant-based pure function

---

Dex223Oracle.sol Test Plan

• Deploy Oracle with valid factory address -- verify constructor succeeds
• Deploy Oracle with address(0) factory -- verify constructor reverts
• Call getSqrtPriceX96(address(0)) -- verify reverts
• Call getSqrtPriceX96 on an uninitialized pool -- verify reverts
• Call getAmountOut(buy, sell, 0) -- verify reverts
• Call getAmountOut with tokens where sqrtPriceX96 > 2^128 -- verify no overflow
• Call getAmountOut where result would be 0 before inverse -- verify reverts
• Call findPoolWithHighestLiquidity with tokenB == address(0) -- verify reverts
• Call feeTiers(0), feeTiers(1), feeTiers(2) -- verify returns 500, 3000, 10000
• Call feeTiers(3) -- verify reverts
• Integration test: Margin Module getPositionStatus() with patched Oracle

---

---

Part 2: OracleLibrary.sol Audit

Summary

Comprehensive security audit of the OracleLibrary library in contracts/dex-periphery/base/OracleLibrary.sol identifying 8 vulnerabilities ranging from High to Low severity. All vulnerabilities have been fixed and verified to compile successfully.

---

V1 - Unsafe int56-to-int24 Truncation in consult() (High)

Severity: High
Location: consult() -- line 34 (original)

Description: The arithmetic mean tick is computed as int24(tickCumulativesDelta / secondsAgo). The division tickCumulativesDelta / secondsAgo is performed on an int56 value divided by a uint32, yielding an int56 result. This int56 is then silently narrowed to int24 via a direct cast. If the true mean tick exceeds the int24 range (-8,388,608 to 8,388,607) -- which can happen with extreme tick cumulative values over short windows -- the high bits are silently discarded, producing a completely wrong tick value. While the Uniswap V3 valid tick range (-887,272 to 887,272) fits within int24, a corrupted intermediate value outside this range would not revert but instead produce a nonsensical price, potentially enabling oracle manipulation or causing downstream financial losses in contracts relying on TWAP prices.

Fix: The division result is first stored in a properly-typed int56 meanTick variable. Before narrowing to int24, a require check validates that meanTick falls within the valid tick range (TickMath.MIN_TICK to TickMath.MAX_TICK). Only after validation does the safe downcast occur.

---

V2 - Division by Zero in consult() for Zero-Liquidity Periods (High)

Severity: High
Location: consult() -- line 40 (original)

Description: The harmonic mean liquidity calculation divides by uint192(secondsPerLiquidityCumulativesDelta) << 32. If the pool had zero in-range liquidity for the entire observation window, secondsPerLiquidityCumulativesDelta will be zero (the cumulative seconds-per-liquidity counter does not advance when liquidity is zero). This produces a division by zero, causing the entire transaction to revert. Any contract that calls consult() on a pool with no in-range liquidity (e.g., a newly created pool, or one where all positions have been removed) will be permanently DOSed. This is particularly dangerous for protocols using TWAP oracles for price feeds -- they could be bricked by an attacker who removes all liquidity from a pool.

Fix: Added an explicit check: if secondsPerLiquidityCumulativesDelta == 0, return harmonicMeanLiquidity = 0 directly (correctly reflecting zero effective liquidity). The division path is only taken when the delta is non-zero.

---

V3 - Division by Zero in getBlockStartingTickAndLiquidity() (Medium)

Severity: Medium
Location: getBlockStartingTickAndLiquidity() -- line 118 (original)

Description: The function computes delta = observationTimestamp - prevObservationTimestamp and then divides by delta on line 119. If two consecutive observations share the same timestamp (which can happen when observationCardinality was just increased and the pool writes a new observation in the same block as the previous one), delta will be zero, causing a division-by-zero revert. This would brick any function that depends on retrieving the block-starting tick for that specific block, including MEV-protection logic and TWAP-based pricing.

Fix: Added require(delta > 0, 'DELTA') to explicitly check that the time delta between observations is non-zero, providing a clear revert reason instead of an opaque EVM panic.

---

V4 - Unsafe int56-to-int24 Truncation in getBlockStartingTickAndLiquidity() (Medium)

Severity: Medium
Location: getBlockStartingTickAndLiquidity() -- line 119 (original)

Description: Same class of vulnerability as V1. The expression int24((tickCumulative - prevTickCumulative) / delta) performs an int56 division and silently truncates to int24. In adversarial conditions (e.g., a manipulated tick cumulative via flash-loan-driven extreme price movement within a single block), the result could exceed int24 range, producing a silently corrupted tick value. This is especially dangerous because getBlockStartingTickAndLiquidity() is typically used in MEV-protection contexts where accuracy is security-critical.

Fix: The division result is stored in int56 meanTick and validated against TickMath.MIN_TICK/TickMath.MAX_TICK before downcasting to int24.

---

V5 - Division by Zero in getWeightedArithmeticMeanTick() (Medium)

Severity: Medium
Location: getWeightedArithmeticMeanTick() -- line 157 (original)

Description: The function divides numerator by int256(denominator) where denominator accumulates all weights. If the input array is empty (length 0) or if all entries have weight == 0, the denominator will be zero, causing a division-by-zero revert with no descriptive error message. While an empty array is an obvious caller bug, the zero-weight scenario is more subtle -- a caller could construct weighted tick data from pool liquidity values, where some or all pools have zero liquidity.

Fix: Added two guards: (1) require(weightedTickData.length > 0, 'EMPTY') at function entry, and (2) require(denominator > 0, 'WGHT') after the accumulation loop to catch zero-weight inputs.

---

V6 - Unsafe int256-to-int24 Truncation in getWeightedArithmeticMeanTick() (Medium)

Severity: Medium
Location: getWeightedArithmeticMeanTick() -- line 157 (original)

Description: The result of numerator / int256(denominator) is an int256 that is directly cast to int24. If the weighted mean tick computation produces a value outside int24 range (e.g., due to adversarial weight manipulation or extreme tick values from misconfigured pool comparisons), the truncation silently discards high bits. The result would be a completely wrong weighted mean price, which could mislead any protocol using this as a multi-pool oracle aggregator.

Fix: The division result is stored in int256 meanTick, then validated with require(meanTick >= int256(TickMath.MIN_TICK) && meanTick <= int256(TickMath.MAX_TICK), 'TICK') before safe downcasting.

---

V7 - Unsigned Integer Underflow in getChainedPrice() (Low)

Severity: Low
Location: getChainedPrice() -- line 173 (original)

Description: The expression tokens.length - 1 is evaluated as an unsigned integer subtraction. If tokens is an empty array (length 0), this underflows to type(uint256).max, which then fails the equality check against ticks.length and reverts with the generic 'DL' error. While the function does eventually revert, the error message is misleading -- it suggests a length mismatch rather than the actual problem (empty input). More critically, if tokens.length == 1, then tokens.length - 1 == 0, and if ticks.length also happens to be 0, the function silently returns syntheticTick = 0 (the loop body never executes), which is semantically incorrect -- a single token cannot have a "chained price".

Fix: Added require(tokens.length >= 2, 'TL') before the existing length check, ensuring the function receives at least one token pair (2 tokens). This makes the underflow impossible and provides a clear error message.

---

V8 - Missing Pool Address Validation (Low)

Severity: Low
Location: consult(), getOldestObservationSecondsAgo(), getBlockStartingTickAndLiquidity()

Description: All three pool-consuming functions accept an address pool parameter but never validate it is non-zero. Calling any of these with address(0) would result in low-level call failures to the zero address, producing opaque EVM revert errors with no useful diagnostic information. While unlikely in well-tested integrations, this makes debugging integration mistakes significantly harder and could mask root causes in complex multi-contract interactions.

Fix: Added require(pool != address(0), 'POOL') at the entry of all three functions: consult(), getOldestObservationSecondsAgo(), and getBlockStartingTickAndLiquidity().

---

Additional Hardening: getOldestObservationSecondsAgo() Timestamp Safety

Location: getOldestObservationSecondsAgo() -- line 87 (original)

Description: The subtraction uint32(block.timestamp) - observationTimestamp could theoretically underflow if observationTimestamp is somehow ahead of block.timestamp (e.g., due to uint32 timestamp wrapping after year 2106, or a chain with non-standard timestamp behavior). Added a defensive require(currentTimestamp >= observationTimestamp, 'FBO') check.

---

Files Changed

File	Changes
contracts/dex-core/Dex223Oracle.sol	9 security fixes for Oracle contract
contracts/dex-periphery/base/OracleLibrary.sol	8 security fixes applied (+47 lines, -14 lines)

Compilation Verification

• Both contracts compile successfully with Solidity 0.7.6 (no new errors or warnings introduced)
• Pre-existing compilation errors in test contracts (MickTimeDex223PoolLib.sol, MockTimeDex223Pool.sol) are unrelated to these changes

OracleLibrary.sol Test Plan

• Verify contract compiles without new errors (confirmed: no new compilation errors introduced)
• Test consult() with valid pool and secondsAgo: should return correct TWAP tick and harmonic mean liquidity
• Test consult() with address(0) pool: should revert with POOL
• Test consult() with secondsAgo = 0: should revert with BP
• Test consult() with a pool that had zero in-range liquidity: should return harmonicMeanLiquidity = 0 without reverting
• Test consult() with extreme tick cumulative values that would exceed int24 range: should revert with TICK
• Test getQuoteAtTick() with boundary ticks (MIN_TICK, MAX_TICK): should return correct quote amounts
• Test getOldestObservationSecondsAgo() with valid pool: should return correct seconds ago
• Test getOldestObservationSecondsAgo() with address(0): should revert with POOL
• Test getOldestObservationSecondsAgo() with uninitialized pool (cardinality 0): should revert with NI
• Test getBlockStartingTickAndLiquidity() with valid pool (no current-block observation): should return slot0 tick and current liquidity
• Test getBlockStartingTickAndLiquidity() with valid pool (current-block observation exists): should compute block-starting tick correctly
• Test getBlockStartingTickAndLiquidity() with address(0): should revert with POOL
• Test getBlockStartingTickAndLiquidity() with same-timestamp observations (delta=0): should revert with DELTA
• Test getBlockStartingTickAndLiquidity() with zero liquidity delta: should return liquidity = 0
• Test getWeightedArithmeticMeanTick() with valid weighted tick data: should return correct weighted mean
• Test getWeightedArithmeticMeanTick() with empty array: should revert with EMPTY
• Test getWeightedArithmeticMeanTick() with all-zero weights: should revert with WGHT
• Test getWeightedArithmeticMeanTick() with extreme tick values that exceed int24 range when averaged: should revert with TICK
• Test getWeightedArithmeticMeanTick() with negative numerator and non-zero remainder: should round toward negative infinity
• Test getChainedPrice() with valid multi-hop route: should return correct synthetic tick
• Test getChainedPrice() with empty tokens array (length 0): should revert with TL
• Test getChainedPrice() with single token (length 1): should revert with TL
• Test getChainedPrice() with mismatched tokens/ticks lengths: should revert with DL
• Test getChainedPrice() with tokens in both sort orders: should correctly add/subtract ticks
• Integration test: full TWAP oracle flow -- create pool, add liquidity, perform swaps, advance time, call consult(), verify price accuracy