diff --git a/src/oracles/UniswapV3Oracle.sol b/src/oracles/UniswapV3Oracle.sol index 261f207..df1d7ad 100644 --- a/src/oracles/UniswapV3Oracle.sol +++ b/src/oracles/UniswapV3Oracle.sol @@ -59,6 +59,18 @@ interface IERC20Decimals { function decimals() external view returns (uint8); } +/// @title IChainlinkAggregatorBounds +/// @notice Circuit-breaker bounds exposed by Chainlink's underlying off-chain aggregator. Mirrors +/// the same interface in {ChainlinkOracle}: a consumer proxy forwards reads to its current +/// `aggregator()`, which carries immutable `minAnswer`/`maxAnswer` bounds. During an extreme +/// dislocation the aggregator clamps its reported answer to a bound, so a price sitting at a +/// bound is a saturated (untrustworthy) reading rather than a true market price. +interface IChainlinkAggregatorBounds { + function aggregator() external view returns (address); + function minAnswer() external view returns (int192); + function maxAnswer() external view returns (int192); +} + /// @title UniswapV3Oracle /// @notice Price oracle using Uniswap V3 TWAP /// @dev Uses time-weighted average prices from Uniswap V3 pools @@ -133,6 +145,12 @@ contract UniswapV3Oracle is IPriceOracle, IPriceOracleAdmin, Ownable { /// @notice Required time the sequencer must have been up before prices are accepted. uint256 public sequencerGracePeriod; + /// @notice Tokens with a configured pool, tracked so {setTwapPeriod} can re-grow every pool's + /// observation buffer when the period is raised (F2). Append-only; entries whose feed was + /// later removed are skipped at use (`poolConfigs[token].pool == 0`). + address[] private _configuredTokens; + mapping(address => bool) private _tokenTracked; + // ═══════════════════════════════════════════════════════════════════════════ // ERRORS // ═══════════════════════════════════════════════════════════════════════════ @@ -147,6 +165,10 @@ contract UniswapV3Oracle is IPriceOracle, IPriceOracleAdmin, Ownable { /// so the TWAP would degenerate toward manipulable spot. `have` < `need`. error InsufficientObservationCardinality(address pool, uint16 have, uint16 need); + /// @notice Quote-token Chainlink answer sits at the aggregator's min/maxAnswer circuit-breaker + /// bound, i.e. the feed is saturated and the value is not a trustworthy market price. + error AnswerOutOfBounds(address token, int256 answer, int192 minAnswer, int192 maxAnswer); + event SequencerUptimeFeedConfigured(address indexed feed, uint256 gracePeriod); // ═══════════════════════════════════════════════════════════════════════════ @@ -289,6 +311,12 @@ contract UniswapV3Oracle is IPriceOracle, IPriceOracleAdmin, Ownable { quoteTokenFeeds[quoteToken] = quoteFeed; } + // F2: remember the token so a later setTwapPeriod can re-validate/grow this pool's buffer. + if (!_tokenTracked[token]) { + _tokenTracked[token] = true; + _configuredTokens.push(token); + } + emit PriceFeedConfigured(token, pool); } @@ -319,10 +347,23 @@ contract UniswapV3Oracle is IPriceOracle, IPriceOracleAdmin, Ownable { } /// @notice Set TWAP observation period + /// @dev F2: a longer period needs a deeper observation ring buffer. `configurePool` grows the + /// buffer for the period in effect at config time, but raising the period here previously + /// skipped that check — an already-configured pool could then no longer span the window and + /// `observe()` would revert (price unavailable). Re-ensure (and opportunistically grow) + /// every configured pool for the new period; reverts `InsufficientObservationCardinality` + /// if a pool cannot be grown to support `period`, so an unsupportable period is rejected. /// @param period New TWAP period in seconds function setTwapPeriod(uint32 period) external onlyOwner { require(period > 0, "Invalid period"); twapPeriod = period; + + uint256 len = _configuredTokens.length; + for (uint256 i = 0; i < len; i++) { + address pool = poolConfigs[_configuredTokens[i]].pool; + if (pool == address(0)) continue; // feed since removed + _ensureObservationCardinality(IUniswapV3Pool(pool)); + } } /// @notice Configure the L2 sequencer uptime feed. Set to `address(0)` to disable on L1. @@ -383,6 +424,12 @@ contract UniswapV3Oracle is IPriceOracle, IPriceOracleAdmin, Ownable { if (answeredInRound < roundId) revert StalePrice(token, updatedAt, maxAge); if (block.timestamp - updatedAt > maxAge) revert StalePrice(token, updatedAt, maxAge); + // F1: circuit breaker — reject a quote answer clamped to the aggregator's min/maxAnswer + // bound. A saturated quote feed during an extreme dislocation would otherwise propagate a + // pinned (floor/ceiling) value through the TWAP→USD conversion, mis-pricing the asset + // (under-/over-valuing exposure and slashing). Parity with ChainlinkOracle. + _checkAnswerBounds(token, quoteFeed, answer); + uint8 feedDecimals = AggregatorV3Interface(quoteFeed).decimals(); if (feedDecimals < 18) { // forge-lint: disable-next-line(unsafe-typecast) @@ -416,6 +463,32 @@ contract UniswapV3Oracle is IPriceOracle, IPriceOracleAdmin, Ownable { data.isValid = true; } + /// @dev Revert when `answer` is pinned to the quote feed's underlying circuit-breaker bound. + /// Bounds live on the proxy's current `aggregator()`, not the proxy, so resolve it first. + /// Feeds that do not expose these getters are skipped via try/catch — the sign/staleness + /// checks at the call site still apply. Identical logic to {ChainlinkOracle._checkAnswerBounds}. + function _checkAnswerBounds(address token, address feed, int256 answer) internal view { + try IChainlinkAggregatorBounds(feed).aggregator() returns (address agg) { + if (agg == address(0)) return; + try IChainlinkAggregatorBounds(agg).minAnswer() returns (int192 minAnswer) { + try IChainlinkAggregatorBounds(agg).maxAnswer() returns (int192 maxAnswer) { + if (minAnswer < maxAnswer && (answer <= minAnswer || answer >= maxAnswer)) { + revert AnswerOutOfBounds(token, answer, minAnswer, maxAnswer); + } + } catch { } + } catch { } + } catch { + // Some proxies expose minAnswer()/maxAnswer() directly without aggregator(). + try IChainlinkAggregatorBounds(feed).minAnswer() returns (int192 minAnswer) { + try IChainlinkAggregatorBounds(feed).maxAnswer() returns (int192 maxAnswer) { + if (minAnswer < maxAnswer && (answer <= minAnswer || answer >= maxAnswer)) { + revert AnswerOutOfBounds(token, answer, minAnswer, maxAnswer); + } + } catch { } + } catch { } + } + } + /// @dev Reverts if the sequencer feed (when configured) reports the L2 sequencer /// as down or recently restarted within `sequencerGracePeriod`. No-op on L1. function _requireSequencerUp() internal view { diff --git a/test/audit/medlow/Oracles.t.sol b/test/audit/medlow/Oracles.t.sol index 1e30578..141c819 100644 --- a/test/audit/medlow/Oracles.t.sol +++ b/test/audit/medlow/Oracles.t.sol @@ -195,7 +195,11 @@ contract ChainlinkOracleBoundsTest is Test { vm.expectRevert( abi.encodeWithSelector( - ChainlinkOracle.AnswerOutOfBounds.selector, address(token), int256(1000e8), int192(1000e8), int192(9000e8) + ChainlinkOracle.AnswerOutOfBounds.selector, + address(token), + int256(1000e8), + int192(1000e8), + int192(9000e8) ) ); oracle.getPrice(address(token)); @@ -208,7 +212,11 @@ contract ChainlinkOracleBoundsTest is Test { vm.expectRevert( abi.encodeWithSelector( - ChainlinkOracle.AnswerOutOfBounds.selector, address(token), int256(9000e8), int192(1000e8), int192(9000e8) + ChainlinkOracle.AnswerOutOfBounds.selector, + address(token), + int256(9000e8), + int192(1000e8), + int192(9000e8) ) ); oracle.getPrice(address(token)); @@ -461,3 +469,122 @@ contract UniswapV3OracleZeroPriceTest is Test { oracle.getPriceData(address(token)); } } + +// ───────────────────────────────────────────────────────────────────────────── +// UniswapV3Oracle: quote-feed min/maxAnswer circuit breaker (F1, medium) +// ───────────────────────────────────────────────────────────────────────────── + +contract UniswapV3OracleQuoteBoundsTest is Test { + UniswapV3Oracle internal oracle; + MockCardinalityPool internal pool; + MockToken internal token; // token0, 6 dec + MockToken internal quote; // token1, 18 dec + MockBoundedAggregator internal quoteFeed; + + function setUp() public { + vm.warp(1_000_000); + token = new MockToken(6); + quote = new MockToken(18); + pool = new MockCardinalityPool(address(token), address(quote), 200, true); + pool.setTick(0); + quoteFeed = new MockBoundedAggregator(8); + quoteFeed.setRound(1, 1); + quoteFeed.setData(1800e8, block.timestamp); + oracle = new UniswapV3Oracle(address(quote)); + oracle.configurePool(address(token), address(pool), address(quoteFeed), false); + } + + /// F1: a quote answer pinned to the aggregator's minAnswer floor is rejected (parity with + /// ChainlinkOracle). Pre-fix the Uniswap quote-feed path had no bounds check, so a saturated + /// quote feed would propagate a pinned floor/ceiling through the TWAP→USD conversion. + function test_F1_RevertWhenQuoteAnswerAtMinBound() public { + quoteFeed.setBounds(int192(1000e8), int192(9000e8)); + quoteFeed.setData(1000e8, block.timestamp); + vm.expectRevert( + abi.encodeWithSelector( + UniswapV3Oracle.AnswerOutOfBounds.selector, + address(token), + int256(1000e8), + int192(1000e8), + int192(9000e8) + ) + ); + oracle.getPrice(address(token)); + } + + function test_F1_RevertWhenQuoteAnswerAtMaxBound() public { + quoteFeed.setBounds(int192(1000e8), int192(9000e8)); + quoteFeed.setData(9000e8, block.timestamp); + vm.expectRevert( + abi.encodeWithSelector( + UniswapV3Oracle.AnswerOutOfBounds.selector, + address(token), + int256(9000e8), + int192(1000e8), + int192(9000e8) + ) + ); + oracle.getPrice(address(token)); + } + + /// A quote answer strictly inside [min, max] still prices normally. + function test_F1_QuoteAnswerWithinBoundsAccepted() public { + quoteFeed.setBounds(int192(1000e8), int192(9000e8)); + quoteFeed.setData(1800e8, block.timestamp); + assertGt(oracle.getPrice(address(token)), 0); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// UniswapV3Oracle: setTwapPeriod re-validates observation cardinality (F2, medium) +// ───────────────────────────────────────────────────────────────────────────── + +contract UniswapV3OracleSetTwapCardinalityTest is Test { + UniswapV3Oracle internal oracle; + MockToken internal token; // 6 dec + MockToken internal quote; // 18 dec + MockBoundedAggregator internal quoteFeed; + + function setUp() public { + vm.warp(1_000_000); + token = new MockToken(6); + quote = new MockToken(18); + quoteFeed = new MockBoundedAggregator(8); + quoteFeed.setRound(1, 1); + quoteFeed.setData(1800e8, block.timestamp); + oracle = new UniswapV3Oracle(address(quote)); + } + + /// F2: raising the TWAP period re-grows an already-configured (growable) pool's observation + /// buffer to span the new window. Pre-fix setTwapPeriod skipped the cardinality check entirely. + function test_F2_SetTwapPeriodGrowsConfiguredPool() public { + MockCardinalityPool growable = new MockCardinalityPool(address(token), address(quote), 200, true); + growable.setTick(0); + // Configures fine at the default 1800s period (needs ceil(1800/12)=150 <= 200). + oracle.configurePool(address(token), address(growable), address(quoteFeed), false); + + // Raise to 36000s -> needs ceil(36000/12) = 3000 slots. + oracle.setTwapPeriod(36_000); + + (,,, uint16 card, uint16 cardNext,,) = growable.slot0(); + assertGe(uint256(card >= cardNext ? card : cardNext), 3000, "pool grown to cover the raised period"); + } + + /// F2: raising the period beyond a non-growable pool's buffer is rejected, instead of silently + /// leaving a pool whose `observe()` over the new window would revert (price unavailable). + function test_F2_SetTwapPeriodRevertsWhenPoolCannotGrow() public { + MockCardinalityPool fixedPool = new MockCardinalityPool(address(token), address(quote), 200, false); + fixedPool.setTick(0); + oracle.configurePool(address(token), address(fixedPool), address(quoteFeed), false); + + vm.expectRevert( + abi.encodeWithSelector( + UniswapV3Oracle.InsufficientObservationCardinality.selector, + address(fixedPool), + uint16(200), + uint16(3000) + ) + ); + oracle.setTwapPeriod(36_000); + } +}