From e3fae8ddc4c20dd7f7e12873ec0ee65454f7901d Mon Sep 17 00:00:00 2001 From: jayesh yadav Date: Tue, 16 Jun 2026 11:34:30 +0530 Subject: [PATCH] fix(oracle): make supply-oracle rate-limit per-time, not per-call The Layer 3 rate-limit clamped the factor's move per commit, but commit is permissionless with no cooldown, so the clamp was not actually a rate limit. A caller could invoke commit repeatedly in a single block, each call reading the freshly written prev factor and advancing another maxFactorDeltaBps toward the median, walking the factor all the way to the median in ceil(distance / maxStep) back-to-back calls inside one block. That collapses the human-reaction window the clamp exists to preserve from several commit intervals to a single block, defeating the stated containment guarantee. Fix: add a minCommitInterval cooldown (default 1 hour) enforced against the stored committed[token].timestamp. A successful commit cannot follow the previous one for the same token until the interval elapses, so the factor moves at most one clamped step per interval no matter how often commit is called. The single prev read is reused for both the cooldown check and the clamp. minCommitInterval is added to setParams with validation (nonzero and at most maxCommitAge) and to the ParamsSet event, and the Layer 3 NatSpec now states the cooldown is load-bearing. Adds test_RateLimit_PerTimeNotPerCommit_BlocksSameBlockWalk, which reproduces the single-block walk and asserts a second same-block commit reverts CommitTooSoon and the next step lands only after the cooldown. 76 tests pass. --- src/oracle/SupplyOracle.sol | 57 ++++++++++++++++++++++++++++++------- test/SupplyOracle.t.sol | 40 +++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/src/oracle/SupplyOracle.sol b/src/oracle/SupplyOracle.sol index b65fcab..4dd5c2a 100644 --- a/src/oracle/SupplyOracle.sol +++ b/src/oracle/SupplyOracle.sol @@ -35,6 +35,12 @@ error SupplyOracle_NotEnoughFreshReports(address token, uint256 fresh, uint256 r /// divergence freeze. The commit reverts, so last-good persists untouched. error SupplyOracle_SourcesDiverged(address token, uint256 spreadBps, uint256 toleranceBps); +/// @notice Thrown when commit is called again before minCommitInterval has +/// elapsed since the token's last successful commit. This makes the rate-limit +/// per-time rather than per-call, so the factor cannot be walked to the median +/// by repeated permissionless commits inside a single block. +error SupplyOracle_CommitTooSoon(address token, uint256 nextAllowedAt); + error SupplyOracle_AlreadyReporter(address reporter); error SupplyOracle_UnknownReporter(address reporter); error SupplyOracle_TooManyReporters(); @@ -66,11 +72,14 @@ error SupplyOracle_InvalidParams(); * constituent without the methodology engine noticing (spec Section 8.2). * * Layer 3, contain. The committed factor moves toward the median by at most - * `maxFactorDeltaBps` per commit, so a correct-but-large change is approached - * gradually and a malicious spike cannot move the index more than one step - * before a human reacts. A hard `maxCommitAge` ceiling fails the read closed - * if every reporter has gone silent for too long, and the guardian can pause - * all reads outright. + * `maxFactorDeltaBps` per commit, and a `minCommitInterval` cooldown spreads + * those commits across time, so the factor moves at most one step per interval + * however often commit is called, and a malicious spike cannot move the index + * more than one step before a human reacts. The cooldown is load-bearing: + * without it, commit being permissionless and cooldown-free would let anyone + * walk the factor to the median with repeated calls in a single block. A hard + * `maxCommitAge` ceiling fails the read closed if every reporter has gone + * silent for too long, and the guardian can pause all reads outright. */ contract SupplyOracle is ISupplyOracle, Ownable2Step { using Math for uint256; @@ -126,6 +135,12 @@ contract SupplyOracle is ISupplyOracle, Ownable2Step { /// @notice Maximum per-commit move of the factor, in bps of the prior factor. uint256 public maxFactorDeltaBps = 1000; + /// @notice Minimum wall-clock seconds between successful commits for a token. + /// Combined with the per-commit clamp, this bounds the factor's rate of + /// change to one maxStep per interval regardless of how often commit is + /// called, which is what gives a human time to react to a malicious move. + uint256 public minCommitInterval = 1 hours; + event GuardianSet(address indexed guardian); event PausedSet(bool paused); event ReporterAdded(address indexed reporter); @@ -137,7 +152,8 @@ contract SupplyOracle is ISupplyOracle, Ownable2Step { uint256 divergenceToleranceBps, uint256 reportStaleAfter, uint256 maxCommitAge, - uint256 maxFactorDeltaBps + uint256 maxFactorDeltaBps, + uint256 minCommitInterval ); modifier onlyReporter() { @@ -212,6 +228,16 @@ contract SupplyOracle is ISupplyOracle, Ownable2Step { function commit(address token) external { if (paused) revert SupplyOracle_Paused(); + // Per-time rate-limit. A token's factor cannot be committed again until + // minCommitInterval has elapsed since its last successful commit. This + // is what turns the per-commit clamp below into an actual rate limit: + // without it, commit being permissionless and cooldown-free would let + // anyone walk the factor to the median with repeated calls in one block. + Committed memory prev = committed[token]; + if (prev.initialized && block.timestamp < uint256(prev.timestamp) + minCommitInterval) { + revert SupplyOracle_CommitTooSoon(token, uint256(prev.timestamp) + minCommitInterval); + } + // Gather fresh reporter values. uint256 n = _reporterList.length; uint256[] memory fresh = new uint256[](n); @@ -249,8 +275,8 @@ contract SupplyOracle is ISupplyOracle, Ownable2Step { } // Layer 3 rate-limit: clamp the move toward the median so a large jump - // is approached over several commits rather than landing at once. - Committed memory prev = committed[token]; + // is approached over several commits, each at least minCommitInterval + // apart (enforced above), rather than landing at once. uint256 next = median; if (prev.initialized) { uint256 maxStep = prev.factorWad.mulDiv(maxFactorDeltaBps, BPS, Math.Rounding.Floor); @@ -320,12 +346,13 @@ contract SupplyOracle is ISupplyOracle, Ownable2Step { uint256 divergenceToleranceBps_, uint256 reportStaleAfter_, uint256 maxCommitAge_, - uint256 maxFactorDeltaBps_ + uint256 maxFactorDeltaBps_, + uint256 minCommitInterval_ ) external onlyOwner { if ( minReporters_ == 0 || minReporters_ > MAX_REPORTERS || divergenceToleranceBps_ > BPS || reportStaleAfter_ == 0 || maxCommitAge_ < reportStaleAfter_ || maxFactorDeltaBps_ == 0 - || maxFactorDeltaBps_ > BPS + || maxFactorDeltaBps_ > BPS || minCommitInterval_ == 0 || minCommitInterval_ > maxCommitAge_ ) { revert SupplyOracle_InvalidParams(); } @@ -334,7 +361,15 @@ contract SupplyOracle is ISupplyOracle, Ownable2Step { reportStaleAfter = reportStaleAfter_; maxCommitAge = maxCommitAge_; maxFactorDeltaBps = maxFactorDeltaBps_; - emit ParamsSet(minReporters_, divergenceToleranceBps_, reportStaleAfter_, maxCommitAge_, maxFactorDeltaBps_); + minCommitInterval = minCommitInterval_; + emit ParamsSet( + minReporters_, + divergenceToleranceBps_, + reportStaleAfter_, + maxCommitAge_, + maxFactorDeltaBps_, + minCommitInterval_ + ); } // ======================================================================== diff --git a/test/SupplyOracle.t.sol b/test/SupplyOracle.t.sol index 16f41a1..53aec0f 100644 --- a/test/SupplyOracle.t.sol +++ b/test/SupplyOracle.t.sol @@ -19,7 +19,8 @@ import { SupplyOracle_SourcesDiverged, SupplyOracle_FactorAboveOne, SupplyOracle_NotReporter, - SupplyOracle_NotGuardian + SupplyOracle_NotGuardian, + SupplyOracle_CommitTooSoon } from "src/oracle/SupplyOracle.sol"; import { MockERC20 } from "test/mocks/MockERC20.sol"; @@ -274,6 +275,43 @@ contract SupplyOracleTest is Test { assertLt(finalFactor, 0.6e18); } + /// @notice The rate-limit must be per-time, not per-call. commit is + /// permissionless, so without a cooldown an attacker could call it + /// repeatedly in one block, each call reading the freshly-written factor + /// and advancing another step, walking it to the median in a single block + /// and erasing the human-reaction window the clamp is meant to preserve. + function test_RateLimit_PerTimeNotPerCommit_BlocksSameBlockWalk() public { + _exclude(treasury); + _exclude(vesting); + _reportAll(0.9e18, 0.9e18, 0.9e18); + oracle.commit(address(token)); // initial commit, factor 0.90 + + // Reporters agree on a large drop. This is the malicious-spike case the + // rate-limit exists to slow down. + vm.warp(block.timestamp + oracle.minCommitInterval()); + _reportAll(0.45e18, 0.45e18, 0.45e18); + oracle.commit(address(token)); + (uint256 afterOne,,) = oracle.freeFloatFactor(address(token)); + assertEq(afterOne, 0.81e18, "first commit should move exactly one clamped step"); + + // A second commit in the SAME block must revert. This is the regression + // guard: previously it would have advanced another step off the just + // written 0.81, and a loop could reach 0.45 within the block. + vm.expectRevert( + abi.encodeWithSelector( + SupplyOracle_CommitTooSoon.selector, address(token), block.timestamp + oracle.minCommitInterval() + ) + ); + oracle.commit(address(token)); + + // The factor only advances once the cooldown has elapsed. + vm.warp(block.timestamp + oracle.minCommitInterval()); + oracle.commit(address(token)); + (uint256 afterTwo,,) = oracle.freeFloatFactor(address(token)); + assertLt(afterTwo, afterOne, "second step should apply only after the cooldown"); + assertEq(afterTwo, afterOne - afterOne * oracle.maxFactorDeltaBps() / 10_000, "not a single clamped step"); + } + function test_HardStaleness_ReadRevertsPastMaxCommitAge() public { _reportAll(0.9e18, 0.9e18, 0.9e18); oracle.commit(address(token));