From b4cef4c11b9a399dba609b45893d44937001f9c6 Mon Sep 17 00:00:00 2001 From: Josh Hardy Date: Mon, 10 Aug 2026 15:21:16 +0000 Subject: [PATCH] fix(dia): require strictly-positive cross-epoch margin + clamp round timestamps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial-pass findings on DIAVaultOracle (mutation-test run @ c371a23). C1 (HIGH) — the cross-epoch invariant #280 called 'airtight' at pauseTimeAfter == maxAge is defeated by DIA feed forward clock skew. Staleness is aged from the push's own SOURCE timestamp, but the pause is measured in block.timestamp; a feed running even 2s fast stamps a pre-split observation 2s after effectiveTime, which then passes staleness at the pause-lift instant and pairs with the post-split ratio — serving 200e8 for a true 100e8 (verified repro). The margin pauseTimeAfter - maxAge is exactly the forward skew a config tolerates, and equality tolerates zero. Fix: init now requires pauseTimeAfter > maxAge STRICTLY (was >=), so a zero-margin config can no longer deploy, and the NatSpec now states the margin must exceed the feed's worst-case forward skew (operators size it; prod's 1h margin is the reference) rather than claiming equality is airtight. New tests: testInitRejectsPauseAfterEqualToMaxAge, testForwardSkewIsRejectedWithinMargin (pins the exact tolerance boundary and the operator-owned residual beyond it); existing equality-boundary tests and configs updated to a positive margin. C3 (LOW) — latestRoundData returned updatedAt/startedAt == the raw push timestamp, which for an accepted future-dated push exceeds block.timestamp and underflow-reverts a Chainlink-style consumer computing block.timestamp - updatedAt. Fix: clamp both to block.timestamp (a fresh future push reports age 0); roundId still tracks the raw timestamp. New test testLatestRoundDataClampsFutureTimestamp. C2/C4/C5 (LOW/doc) — corrected NatSpec that was contradicted by the code: roundId is a freshness token, NOT monotonic (it regresses if DIA republishes a lower source timestamp — diff for inequality, don't assert >); a uint256-range overflow surfaces as LibDecimalFloat's FixedDecimalOverflow, not this contract's VaultSharePriceOverflow; and a non-zero ERC-4626 decimals offset is mispriced (not merely un-pausable), a second reason arbitrary vaults are unsupported. script/ untouched. 191 tests green; fmt/slither/reuse/single-contract clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013hi8wq9YkjWPcGACXCEKeL --- src/concrete/oracle/DIAVaultOracle.sol | 159 ++++++++++++------ .../deploy/DIADeployerSaltDerivation.t.sol | 2 +- .../DIAVaultOracleBeaconSetDeployer.t.sol | 4 +- test/src/concrete/oracle/DIAVaultOracle.t.sol | 143 ++++++++++++---- test/src/e2e/DIAStackE2E.t.sol | 4 +- test/src/fork/DIAVaultOracleForkBase.t.sol | 12 +- test/src/script/Deploy.t.sol | 2 +- 7 files changed, 233 insertions(+), 93 deletions(-) diff --git a/src/concrete/oracle/DIAVaultOracle.sol b/src/concrete/oracle/DIAVaultOracle.sol index 260414f..91a8414 100644 --- a/src/concrete/oracle/DIAVaultOracle.sol +++ b/src/concrete/oracle/DIAVaultOracle.sol @@ -38,12 +38,15 @@ error ZeroCorporateActionsVault(); /// required, else the auto-pause never fires despite being "configured". error InvalidPauseConfig(); -/// @dev Error raised when `pauseTimeAfter < maxAge`. The post-action pause MUST -/// last at least as long as the DIA staleness window, otherwise a +/// @dev Error raised when `pauseTimeAfter <= maxAge`. The post-action pause MUST +/// last STRICTLY longer than the DIA staleness window, otherwise a /// stale-but-not-yet-`maxAge` pre-action DIA price can be served against the /// already-rebalanced post-action vault ratio once the pause lifts — mispricing -/// collateral across a corporate-action boundary. See the contract NatSpec -/// ("Cross-epoch safety invariant") for the full argument. +/// collateral across a corporate-action boundary. Equality (`pauseTimeAfter == +/// maxAge`) tolerates zero forward feed clock skew and is rejected; the margin +/// `pauseTimeAfter - maxAge` must exceed the DIA feed's worst-case forward skew. +/// See the contract NatSpec ("Cross-epoch safety invariant") for the full +/// argument. /// @param pauseTimeAfter The configured post-action pause (seconds). /// @param maxAge The configured DIA staleness window (seconds). error PauseTimeAfterBelowMaxAge(uint256 pauseTimeAfter, uint256 maxAge); @@ -72,7 +75,12 @@ error ZeroVaultSupply(); /// price is never a valid Chainlink-compatible answer. error ZeroVaultSharePrice(); -/// @dev Error raised when the vault share price overflows int256. +/// @dev Error raised when the vault share price, though it fits uint256, +/// exceeds `int256.max` and so cannot be returned as the signed Chainlink +/// answer. A price so large it overflows uint256 during the 8-decimal scaling +/// aborts EARLIER inside `LibDecimalFloat` with its own `FixedDecimalOverflow` +/// error, not this one — both are fail-closed, but only this int256-band case +/// carries the contract's own selector. /// @param price8 The unsigned 8-decimal share price that wouldn't fit. error VaultSharePriceOverflow(uint256 price8); @@ -105,13 +113,17 @@ error HistoricalRoundDataUnsupported(uint80 roundId); /// start pausing. /// @param pauseTimeAfter Seconds after a completed action's `effectiveTime` to /// keep pausing. At least one of before/after must be non-zero, AND -/// `pauseTimeAfter >= maxAge` is REQUIRED and enforced at init +/// `pauseTimeAfter > maxAge` (STRICTLY) is REQUIRED and enforced at init /// (`PauseTimeAfterBelowMaxAge`) — the post-action pause must outlast the DIA /// staleness window so a pre-action price can never be served against the -/// post-action ratio. The exact-equality boundary (`pauseTimeAfter == maxAge`) -/// is safe: the staleness check rejects the `maxAge` edge, so at pause-lift the -/// oldest still-acceptable push is strictly newer than `effectiveTime`. A -/// margin above `maxAge` is still recommended as defence-in-depth. +/// post-action ratio. The margin `pauseTimeAfter - maxAge` is the maximum +/// forward DIA feed clock skew this config tolerates: because staleness is aged +/// from the push's own (skewable) source timestamp, a feed running `skew` +/// seconds fast can present a pre-action observation stamped up to `skew` after +/// `effectiveTime`, and only a margin exceeding `skew` guarantees every +/// still-acceptable push at pause-lift was observed at or after `effectiveTime`. +/// Size the margin above the feed's worst-case forward skew (the prod config's +/// 1h margin over a 2h `maxAge` is the reference). /// @dev The corporate-actions vault is NOT a config field: it is derived as /// `IERC4626(vault).asset()` — the tStock the wtStock wraps, which is the /// contract that implements `ICorporateActionsV1`. Deriving it removes a @@ -179,9 +191,15 @@ struct DIAVaultOracleConfig { /// handled by the mandatory auto-pause below. /// /// Pointing this oracle at an arbitrary third-party ERC-4626 remains -/// unsupported — not because of donations, but because nothing outside the -/// ST0x stack guarantees the corporate-action wiring the auto-pause depends -/// on. See `_vaultSharePrice` for the per-function note. +/// unsupported, for TWO reasons. First, nothing outside the ST0x stack +/// guarantees the corporate-action wiring the auto-pause depends on. Second, +/// `_vaultSharePrice` uses the RAW `totalAssets/totalSupply` as assets-per- +/// share, which only holds when the vault's share decimals equal its asset +/// decimals (a zero ERC-4626 decimals offset, as the production `wtStock` +/// has). A vault with a non-zero decimals offset is mispriced by `10^offset` +/// in one orientation and bricks (`ZeroVaultSharePrice`) in the other — so +/// "unsupported" here means mispriced, not merely un-pausable. See +/// `_vaultSharePrice` for the per-function note. /// /// Auto-pause: on every read the oracle consults `ICorporateActionsV1` on the /// corporate-actions vault — derived as the priced vault's `asset()`, i.e. the @@ -194,26 +212,34 @@ struct DIAVaultOracleConfig { /// admin: config is immutable, set once at initialize; to change anything, /// deploy a fresh proxy and migrate consumers. /// -/// Cross-epoch safety invariant (`pauseTimeAfter >= maxAge`, enforced at init): -/// the share price multiplies a DIA equity price by the vault's LIVE +/// Cross-epoch safety invariant (`pauseTimeAfter > maxAge` STRICTLY, enforced at +/// init): the share price multiplies a DIA equity price by the vault's LIVE /// `totalAssets/totalSupply` ratio. Those two inputs must belong to the same /// corporate-action epoch. When an action completes, the vault ratio rebalances /// atomically, but DIA keeps serving the pre-action equity price until its next /// push — up to `maxAge` seconds. The post-window pause is the only barrier -/// between the two epochs. If `pauseTimeAfter < maxAge` the pause lifts while a -/// pre-action DIA price is still within `maxAge` (hence accepted by the -/// staleness check), and that stale price pairs with the already-rebalanced -/// ratio: on a 2:1 split the share is valued at ~2x, letting a borrower draw -/// against phantom collateral (bad debt). Requiring `pauseTimeAfter >= maxAge` -/// makes the oldest still-acceptable push at pause-lift STRICTLY NEWER than the -/// action's `effectiveTime`: the staleness check rejects the exact-`maxAge` -/// edge (`age >= maxAge` is stale), so at the pause-lift instant -/// `t = effectiveTime + pauseTimeAfter` any served push is timestamped -/// `> t - maxAge >= effectiveTime`. The equal boundary (`pauseTimeAfter == -/// maxAge`) is therefore airtight — no same-instant ambiguity — so only -/// same-epoch (post-action) prices are ever served. The staleness check alone -/// is NOT sufficient — it bounds age, not epoch; the invariant plus the -/// edge-rejecting staleness together are what close the window. +/// between the two epochs. If the pause lifts while a pre-action DIA price is +/// still within `maxAge` (hence accepted by the staleness check), that stale +/// price pairs with the already-rebalanced ratio: on a 2:1 split the share is +/// valued at ~2x, letting a borrower draw against phantom collateral (bad debt). +/// +/// The subtlety is which CLOCK ages the push. Staleness is measured from the +/// push's own DIA SOURCE timestamp, not from when it landed on chain, and that +/// source clock is outside our control. A feed running `skew` seconds fast +/// stamps a pre-action observation as far as `skew` AFTER `effectiveTime`, so at +/// the pause-lift instant `t = effectiveTime + pauseTimeAfter` a served push is +/// only guaranteed timestamped `> t - maxAge`, i.e. `> effectiveTime + +/// (pauseTimeAfter - maxAge)` in source time — which corresponds to a real +/// OBSERVATION at or after `effectiveTime` only when the margin `pauseTimeAfter +/// - maxAge` exceeds the feed's forward skew. Equality (`pauseTimeAfter == +/// maxAge`) leaves a zero margin: a feed even one second fast reopens the +/// window (a 2s skew serves the pre-split price at 2x — verified). Init +/// therefore REQUIRES a strictly positive margin (`pauseTimeAfter > maxAge`), +/// and operators MUST size that margin above their feed's worst-case forward +/// clock skew — the strict check is the enforceable floor, not a guarantee that +/// any positive margin suffices. The staleness check alone is NOT sufficient — +/// it bounds age, not epoch; the invariant, the edge-rejecting staleness, and a +/// skew-covering margin together are what close the window. /// /// Deployed as a beacon-proxy clone via `ICloneableV2.initialize`. contract DIAVaultOracle is AggregatorV2V3Interface, ICloneableV2, Initializable { @@ -335,18 +361,30 @@ contract DIAVaultOracle is AggregatorV2V3Interface, ICloneableV2, Initializable revert InvalidPauseConfig(); } - // Cross-epoch safety invariant: the post-action pause must outlast the - // DIA staleness window. The vault's NAV ratio rebalances the instant a - // corporate action completes, but DIA may still serve the pre-action - // equity price for up to `maxAge` seconds afterwards. The pause is the - // only thing separating those two epochs; if it lifts while a pre-action - // price is still "fresh" (`pauseTimeAfter < maxAge`), that price pairs - // with the post-action ratio and misprices the share (e.g. ~2x on a 2:1 - // split → over-borrow → bad debt). `pauseTimeAfter >= maxAge` guarantees - // that once the pause lifts, the oldest still-acceptable DIA push was - // timestamped at or after the action's `effectiveTime`. See the - // contract NatSpec for the full argument. - if (config.pauseTimeAfter < config.maxAge) { + // Cross-epoch safety invariant: the post-action pause must STRICTLY + // outlast the DIA staleness window. The vault's NAV ratio rebalances the + // instant a corporate action completes, but DIA may still serve the + // pre-action equity price for up to `maxAge` seconds afterwards. The + // pause is the only thing separating those two epochs; if it lifts while + // a pre-action price is still "fresh" that price pairs with the + // post-action ratio and misprices the share (e.g. ~2x on a 2:1 split → + // over-borrow → bad debt). + // + // The staleness age is measured from the DIA push's OWN source + // timestamp, NOT from when it landed on chain, so a feed whose clock + // runs forward by `skew` stamps a pre-action observation as far as + // `skew` seconds AFTER `effectiveTime` — and that push is then accepted + // for the whole `maxAge` window past its (skewed) timestamp. The margin + // `pauseTimeAfter - maxAge` is exactly the forward feed skew this config + // tolerates: at pause-lift the oldest still-acceptable push was OBSERVED + // at or after `effectiveTime` only if that margin exceeds the feed's max + // forward skew. `>=` (equality) tolerates ZERO skew and is therefore + // rejected — a strictly positive margin is required, and operators MUST + // size it above their feed's worst-case forward clock error (the prod + // config's 1h margin over a 2h maxAge is the reference). See the + // contract NatSpec ("Cross-epoch safety invariant") for the full + // argument. + if (config.pauseTimeAfter <= config.maxAge) { revert PauseTimeAfterBelowMaxAge(config.pauseTimeAfter, config.maxAge); } @@ -405,11 +443,21 @@ contract DIAVaultOracle is AggregatorV2V3Interface, ICloneableV2, Initializable /// @inheritdoc AggregatorV2V3Interface /// @dev `roundId` and `answeredInRound` are derived from the DIA push - /// `timestamp` (truncated to `uint80`) so they advance monotonically per - /// Chainlink convention without adding storage. Integrators that diff - /// `roundId` between calls to detect a fresh update will see a different - /// value whenever DIA has produced a new push. The `uint80` window - /// covers every plausible deployment lifetime. + /// `timestamp` (truncated to `uint80`) so they change whenever DIA produces + /// a new push, without adding storage. They are a FRESHNESS token, not a + /// strictly monotonic counter: if DIA ever republishes a lower source + /// timestamp (a corrected push, a source-clock regression) the id moves + /// backwards, so integrators should diff for inequality — NOT assert + /// `roundId > lastSeen`. The `uint80` window covers every plausible + /// deployment lifetime. + /// + /// `startedAt`/`updatedAt` are the push's source timestamp CLAMPED to + /// `block.timestamp`. `_readDIAChecked` deliberately accepts a future-dated + /// push (a feed running slightly ahead) as fresh; returning that raw + /// future timestamp here would make a Chainlink-style consumer computing + /// `block.timestamp - updatedAt` underflow-revert. Clamping reports such a + /// fresh push as age 0 — which is what "fresh" means — and never emits a + /// timestamp ahead of the block clock. function latestRoundData() external view @@ -421,7 +469,14 @@ contract DIAVaultOracle is AggregatorV2V3Interface, ICloneableV2, Initializable int256 scaledPrice = _vaultSharePrice(diaPrice); uint80 round = uint80(timestamp); - return (round, scaledPrice, timestamp, timestamp, round); + // Slither `timestamp` FALSE POSITIVE: block.timestamp is used only to + // CLAMP the reported push time to the local clock (so a future-dated + // push never reports an age below zero to a consumer), not for any value + // or authorisation decision. Proposer drift on block.timestamp only + // shifts the clamp point by seconds. + // slither-disable-next-line timestamp + uint256 reportedAt = uint256(timestamp) > block.timestamp ? block.timestamp : uint256(timestamp); + return (round, scaledPrice, reportedAt, reportedAt, round); } /// @inheritdoc AggregatorV2V3Interface @@ -468,10 +523,12 @@ contract DIAVaultOracle is AggregatorV2V3Interface, ICloneableV2, Initializable // cannot disambiguate from `DIAPriceStale` / `DIAPriceNotSet`. // // The staleness edge fails closed (`>=`): a push exactly `maxAge` old is - // STALE. This is deliberate — it makes the cross-epoch invariant - // (`pauseTimeAfter >= maxAge`, see the contract NatSpec) airtight at the - // exact-equality boundary, and matches the fail-closed staleness - // convention (the edge counts as stale). + // STALE. This is deliberate — it tightens the cross-epoch invariant by + // one second (see the contract NatSpec) and matches the fail-closed + // staleness convention (the edge counts as stale). It does NOT on its + // own make the invariant "airtight" — the margin `pauseTimeAfter - + // maxAge` must still cover the feed's forward source-clock skew, which + // is why init requires that margin to be strictly positive. // slither-disable-next-line timestamp if (uint256(timestamp) <= block.timestamp && block.timestamp - uint256(timestamp) >= $.maxAge) { revert DIAPriceStale(uint256(timestamp)); diff --git a/test/src/concrete/deploy/DIADeployerSaltDerivation.t.sol b/test/src/concrete/deploy/DIADeployerSaltDerivation.t.sol index 197b42e..90249ec 100644 --- a/test/src/concrete/deploy/DIADeployerSaltDerivation.t.sol +++ b/test/src/concrete/deploy/DIADeployerSaltDerivation.t.sol @@ -56,7 +56,7 @@ contract DIADeployerSaltDerivationTest is Test { maxAge: MAX_AGE, actionTypeMask: ACTION_TYPE_STOCK_SPLIT_V1, pauseTimeBefore: 3600, - pauseTimeAfter: 3600 + pauseTimeAfter: 7200 // > maxAge (strict cross-epoch margin) }); } diff --git a/test/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.t.sol b/test/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.t.sol index 4cd8b11..efdfada 100644 --- a/test/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.t.sol +++ b/test/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.t.sol @@ -58,7 +58,7 @@ contract DIAVaultOracleBeaconSetDeployerTest is Test { maxAge: MAX_AGE, actionTypeMask: ACTION_TYPE_STOCK_SPLIT_V1, pauseTimeBefore: 3600, - pauseTimeAfter: 3600 + pauseTimeAfter: 7200 // > maxAge (strict cross-epoch margin) }); } @@ -112,7 +112,7 @@ contract DIAVaultOracleBeaconSetDeployerTest is Test { // Differing config → different deterministic address. Vary // `pauseTimeAfter` (not `maxAge`) so the cross-epoch invariant - // `pauseTimeAfter >= maxAge` still holds for the second config. + // `pauseTimeAfter > maxAge` still holds for the second config. DIAVaultOracleConfig memory other = _defaultOracleConfig(); other.pauseTimeAfter = _defaultOracleConfig().pauseTimeAfter + 1; DIAVaultOracle second = bsd.newDIAVaultOracle(other); diff --git a/test/src/concrete/oracle/DIAVaultOracle.t.sol b/test/src/concrete/oracle/DIAVaultOracle.t.sol index 1125ca1..ac97a9b 100644 --- a/test/src/concrete/oracle/DIAVaultOracle.t.sol +++ b/test/src/concrete/oracle/DIAVaultOracle.t.sol @@ -44,7 +44,10 @@ contract DIAVaultOracleTest is Test { string internal constant SYMBOL = "COIN"; uint256 internal constant MAX_AGE = 1 hours; uint64 internal constant PAUSE_BEFORE = 3600; - uint64 internal constant PAUSE_AFTER = 3600; + // Strictly greater than MAX_AGE: the cross-epoch invariant now requires a + // positive margin (`pauseTimeAfter > maxAge`) to cover DIA feed forward + // clock skew, so the default config carries a 1h margin over the 1h maxAge. + uint64 internal constant PAUSE_AFTER = 2 hours; event DIAVaultOracleInitialized(address indexed sender, DIAVaultOracleConfig config); @@ -129,7 +132,7 @@ contract DIAVaultOracleTest is Test { } /// @notice A pre-window-ONLY config (`pauseTimeAfter == 0`) is REJECTED: it - /// violates the cross-epoch invariant `pauseTimeAfter >= maxAge`. With no + /// violates the cross-epoch invariant `pauseTimeAfter > maxAge`. With no /// post-window, the oracle would resume serving the instant a split /// completes, pairing a still-fresh pre-split DIA price with the /// already-rebalanced post-split ratio — the exact mispricing the invariant @@ -145,8 +148,8 @@ contract DIAVaultOracleTest is Test { } /// @notice A post-window-only config (`pauseTimeBefore == 0`) is valid as - /// long as `pauseTimeAfter >= maxAge`. The default `PAUSE_AFTER == MAX_AGE` - /// sits exactly on the boundary, which init accepts. + /// long as `pauseTimeAfter > maxAge`. The default `PAUSE_AFTER` carries a + /// positive margin over `MAX_AGE`, which init accepts. function testInitSucceedsWithOnlyPostWindow() external { DIAVaultOracleConfig memory config = _defaultConfig(); config.pauseTimeBefore = 0; @@ -159,8 +162,8 @@ contract DIAVaultOracleTest is Test { } /// @notice The cross-epoch invariant is enforced at init: `pauseTimeAfter` - /// strictly below `maxAge` reverts `PauseTimeAfterBelowMaxAge`, and the - /// exact boundary `pauseTimeAfter == maxAge` is accepted. + /// below `maxAge` reverts `PauseTimeAfterBelowMaxAge`, and one second above + /// `maxAge` (the minimum positive margin) is accepted. function testInitRevertsWhenPauseAfterBelowMaxAge() external { DIAVaultOracleConfig memory config = _defaultConfig(); config.maxAge = 2 hours; @@ -170,15 +173,25 @@ contract DIAVaultOracleTest is Test { abi.encodeWithSelector(PauseTimeAfterBelowMaxAge.selector, uint256(2 hours) - 1, uint256(2 hours)) ); oracle.initialize(abi.encode(config)); + + // One second of margin over maxAge is the minimum init accepts. + config.pauseTimeAfter = uint64(2 hours) + 1; + DIAVaultOracle ok = _deployUninit(); + assertEq(ok.initialize(abi.encode(config)), ICLONEABLE_V2_SUCCESS, "strictly-positive margin is accepted"); } - function testInitAcceptsPauseAfterEqualToMaxAge() external { + /// @notice `pauseTimeAfter == maxAge` (zero margin) is REJECTED. A zero + /// margin tolerates zero forward feed clock skew, so a feed running even one + /// second fast could serve a pre-action price at pause-lift — the exact + /// cross-epoch mispricing this invariant closes. Init requires a strictly + /// positive margin. + function testInitRejectsPauseAfterEqualToMaxAge() external { DIAVaultOracleConfig memory config = _defaultConfig(); config.maxAge = 2 hours; - config.pauseTimeAfter = uint64(2 hours); // exactly on the boundary + config.pauseTimeAfter = uint64(2 hours); // exactly on the boundary — now rejected DIAVaultOracle oracle = _deployUninit(); - bytes32 ok = oracle.initialize(abi.encode(config)); - assertEq(ok, ICLONEABLE_V2_SUCCESS, "pauseTimeAfter == maxAge is on the safe boundary"); + vm.expectRevert(abi.encodeWithSelector(PauseTimeAfterBelowMaxAge.selector, uint256(2 hours), uint256(2 hours))); + oracle.initialize(abi.encode(config)); } /// @notice A corporate-actions vault whose facet reverts must fail the @@ -697,18 +710,16 @@ contract DIAVaultOracleTest is Test { /// post-window is inclusive) the pause gate rejects the read; /// - at `effectiveTime + pauseTimeAfter + 1` (the first unpaused instant) /// the pause is off, but the push is now aged `pauseTimeAfter + 1`, which - /// under the enforced `pauseTimeAfter >= maxAge` exceeds `maxAge`, so the + /// under the enforced `pauseTimeAfter > maxAge` exceeds `maxAge`, so the /// staleness check rejects it. /// - /// A gap here is exactly the HIGH this branch fixes: serving a pre-split - /// price after a 2:1 split reads 2x the true value and mints bad debt in - /// downstream lending markets. + /// A gap here is exactly the HIGH this file's fix addresses: serving a + /// pre-split price after a 2:1 split reads 2x the true value and mints bad + /// debt in downstream lending markets. /// - /// Note this concrete case documents the handover mechanics readably but - /// sits one second off the tight edge (the inclusive post-window leaves a - /// second of slack at `pauseTimeAfter == maxAge`). The tight guarantee — no - /// servable instant for ANY valid config — is fuzzed in - /// `testFuzzPreActionPriceNeverServed` below; both are needed. + /// This concrete case pins pushes stamped at or before `effectiveTime`; + /// the all-config version is fuzzed in `testFuzzPreActionPriceNeverServed`, + /// and the forward-skew case in `testForwardSkewIsRejectedWithinMargin`. function testPreActionPriceNeverServedAcrossPauseHandover() external { DIAVaultOracle oracle = _deployProxy(_defaultConfig()); vault.setTotalAssets(1e18); @@ -741,13 +752,16 @@ contract DIAVaultOracleTest is Test { /// push timestamped at or before a completed action's `effectiveTime` is /// never served — the read always reverts, either paused or stale. /// - /// This is the property the `pauseTimeAfter >= maxAge` init check exists to - /// buy, and fuzzing it is what makes it airtight: the concrete handover test - /// above only pins the default 1h/1h config, where the inclusive post-window - /// leaves a second of slack. Here `pauseTimeAfter` is driven right down onto - /// `maxAge` and the read swept across the whole paused-to-stale transition, - /// so any widening of the staleness edge or narrowing of the pause window - /// opens a servable instant and fails this test. + /// This is the property the `pauseTimeAfter > maxAge` init check exists to + /// buy for pushes observed no later than the action. Fuzzing it sweeps the + /// read across the whole paused-to-stale transition at a tight positive + /// margin, so any widening of the staleness edge or narrowing of the pause + /// window opens a servable instant and fails this test. + /// + /// Scope: the push here is stamped at or BEFORE `effectiveTime`. A push + /// stamped AFTER `effectiveTime` by forward feed clock skew is a separate + /// case whose tolerance is the config margin — see + /// `testForwardSkewIsRejectedWithinMargin`. /// /// Reverting is the whole assertion: a returned price at any point in this /// range is a pre-action equity price paired with a post-action NAV ratio. @@ -759,10 +773,11 @@ contract DIAVaultOracleTest is Test { uint64 elapsed ) external { maxAgeSeconds = uint64(bound(maxAgeSeconds, 1, 30 days)); - // `pauseTimeAfter >= maxAge` is the enforced invariant. Keep the margin - // TIGHT: the property can only break where the two windows meet, and a - // wide margin is the trivially-safe case the fuzzer would waste runs on. - extraPause = uint64(bound(extraPause, 0, 3)); + // `pauseTimeAfter > maxAge` (strict) is the enforced invariant, so the + // margin is at least 1. Keep it TIGHT: the property can only break where + // the two windows meet, and a wide margin is the trivially-safe case the + // fuzzer would waste runs on. + extraPause = uint64(bound(extraPause, 1, 4)); uint64 pauseAfter = maxAgeSeconds + extraPause; pauseBefore = uint64(bound(pauseBefore, 0, 30 days)); // The push is pre-action: at or just before `effectiveTime`. Pushes far @@ -802,6 +817,54 @@ contract DIAVaultOracleTest is Test { ); } + /// @notice The cross-epoch fix in its precise form: the config margin + /// (`pauseTimeAfter - maxAge`) is exactly the forward feed clock skew the + /// deployment tolerates. A pre-action push stamped up to `margin` seconds + /// AFTER `effectiveTime` (as a fast feed would stamp a last pre-split + /// observation) is rejected at pause-lift; a push skewed BEYOND the margin + /// is the residual the operator must size the margin against. + /// + /// This is what enforcing a strictly-positive margin buys, and why equality + /// (zero margin, zero skew tolerance) is rejected at init. With margin M, a + /// push stamped `E + δ` has age `pauseTimeAfter + 1 - δ` at the first + /// unpaused instant, which is `>= maxAge` (stale) exactly while `δ <= M + 1`. + function testForwardSkewIsRejectedWithinMargin() external { + // maxAge 1h, pauseAfter 1h + 10s: this config tolerates ~10s of skew. + uint64 maxAge = 1 hours; + uint64 margin = 10; + DIAVaultOracleConfig memory config = _defaultConfig(); + config.maxAge = maxAge; + config.pauseTimeAfter = maxAge + margin; + + uint64 E = uint64(block.timestamp); + + // A pre-action observation stamped `margin` seconds ahead by a fast + // feed. Within tolerance -> must be rejected (stale) at pause-lift. + DIAVaultOracle o1 = _deployProxy(config); + vault.setTotalAssets(1e18); + vault.setTotalSupply(1e18); + actions.setLatestCompleted(1, ACTION_TYPE_STOCK_SPLIT_V1, E); + diaOracle.setValue(SYMBOL, 100e18, uint128(uint256(E) + margin)); + vault.setTotalAssets(2e18); // 2:1 split + vm.warp(uint256(E) + uint256(config.pauseTimeAfter) + 1); + (bool served,) = address(o1).staticcall(abi.encodeCall(DIAVaultOracle.latestAnswer, ())); + assertFalse(served, "a push skewed within the margin must be rejected, not served at 2x"); + + // Skewed BEYOND the margin (margin + 2): this is the documented residual + // — the operator's margin was too small for this feed's skew, so the + // stale pre-split price IS served. Pinning it keeps the tolerance + // boundary honest rather than implying any positive margin is safe. + vm.warp(E); + DIAVaultOracle o2 = _deployProxy(config); + vault.setTotalAssets(1e18); + vault.setTotalSupply(1e18); + actions.setLatestCompleted(1, ACTION_TYPE_STOCK_SPLIT_V1, E); + diaOracle.setValue(SYMBOL, 100e18, uint128(uint256(E) + margin + 2)); + vault.setTotalAssets(2e18); + vm.warp(uint256(E) + uint256(config.pauseTimeAfter) + 1); + assertEq(o2.latestAnswer(), int256(200e8), "skew beyond the margin is the operator-owned residual"); + } + /// @notice `_readDIAChecked` reverts `DIAPriceNotSet` when the DIA value is /// zero even if the timestamp is non-zero — the value-zero and timestamp-zero /// terms of the not-set check are independent, so a mutant dropping the @@ -912,6 +975,26 @@ contract DIAVaultOracleTest is Test { assertEq(updatedAt, uint256(timestamp)); } + /// @notice A future-dated DIA push (a feed running ahead — accepted as fresh + /// by `_readDIAChecked`) must NOT propagate a future `updatedAt`/`startedAt` + /// through `latestRoundData`: they are clamped to `block.timestamp` so a + /// Chainlink-style consumer computing `block.timestamp - updatedAt` reads + /// age 0 instead of underflow-reverting. `roundId` still reflects the raw + /// push timestamp (freshness token). Issue: C3. + function testLatestRoundDataClampsFutureTimestamp() external { + DIAVaultOracle oracle = _deployProxy(_defaultConfig()); + uint128 futureTs = uint128(block.timestamp + 100); + diaOracle.setValue(SYMBOL, 100e18, futureTs); + vault.setTotalAssets(1e18); + vault.setTotalSupply(1e18); + + (uint80 roundId,, uint256 startedAt, uint256 updatedAt,) = oracle.latestRoundData(); + assertEq(updatedAt, block.timestamp, "updatedAt clamped to now, never ahead of the block clock"); + assertEq(startedAt, block.timestamp, "startedAt clamped identically"); + assertLe(updatedAt, block.timestamp, "updatedAt must never exceed block.timestamp"); + assertEq(uint256(roundId), uint256(futureTs), "roundId still tracks the raw push timestamp"); + } + function testLatestRoundDataMatchesLatestAnswer() external { DIAVaultOracle oracle = _deployProxy(_defaultConfig()); diaOracle.setValue(SYMBOL, 123e18, uint128(block.timestamp)); diff --git a/test/src/e2e/DIAStackE2E.t.sol b/test/src/e2e/DIAStackE2E.t.sol index 3ece117..7e62c9d 100644 --- a/test/src/e2e/DIAStackE2E.t.sol +++ b/test/src/e2e/DIAStackE2E.t.sol @@ -37,8 +37,8 @@ contract DIAStackE2ETest is Test { string internal constant SYMBOL = "COIN"; uint256 internal constant MAX_AGE = 2 hours; uint64 internal constant PAUSE_BEFORE = 1 hours; - // Cross-epoch invariant: pauseTimeAfter >= maxAge. - uint64 internal constant PAUSE_AFTER = 2 hours; + // Cross-epoch invariant: pauseTimeAfter > maxAge (strict, positive margin). + uint64 internal constant PAUSE_AFTER = 3 hours; function setUp() public { diaOracle = new MockDIAOracle(); diff --git a/test/src/fork/DIAVaultOracleForkBase.t.sol b/test/src/fork/DIAVaultOracleForkBase.t.sol index 2c6b4b3..5bc9c2a 100644 --- a/test/src/fork/DIAVaultOracleForkBase.t.sol +++ b/test/src/fork/DIAVaultOracleForkBase.t.sol @@ -49,13 +49,13 @@ contract DIAVaultOracleForkBaseTest is Test { address constant TCOIN = LibProdTokensBase.COIN_RECEIPT_VAULT; // receipt vault, ICorporateActionsV1 uint64 constant PAUSE_BEFORE = 1 hours; - // Satisfies the cross-epoch invariant `pauseTimeAfter >= maxAge`. The DIA - // feed is mocked to a fresh value for the pre-schedule read (see - // `_seedFreshDIA`), so `maxAge` no longer depends on how recently the LIVE - // DIA `COIN` feed was pushed at the fork block — the fork test's real - // subject is the corporate-action vault, not DIA. + // Satisfies the cross-epoch invariant `pauseTimeAfter > maxAge` (strict, with + // a positive skew margin). The DIA feed is mocked to a fresh value for the + // pre-schedule read (see `_seedFreshDIA`), so `maxAge` no longer depends on + // how recently the LIVE DIA `COIN` feed was pushed at the fork block — the + // fork test's real subject is the corporate-action vault, not DIA. uint256 constant MAX_AGE = 1 hours; - uint64 constant PAUSE_AFTER = 1 hours; + uint64 constant PAUSE_AFTER = 2 hours; function _deployOracle() internal returns (DIAVaultOracle) { DIAVaultOracleBeaconSetDeployer bsd = new DIAVaultOracleBeaconSetDeployer( diff --git a/test/src/script/Deploy.t.sol b/test/src/script/Deploy.t.sol index 7ae9246..5e3970e 100644 --- a/test/src/script/Deploy.t.sol +++ b/test/src/script/Deploy.t.sol @@ -76,7 +76,7 @@ contract DeployTest is Test { maxAge: 1 hours, actionTypeMask: ACTION_TYPE_STOCK_SPLIT_V1, pauseTimeBefore: 3600, - pauseTimeAfter: 3600 + pauseTimeAfter: 7200 // > maxAge (strict cross-epoch margin) }); }