Skip to content

fix(dia): enforce pauseTimeAfter >= maxAge (cross-epoch mispricing) - #280

Merged
hardyjosh merged 3 commits into
mainfrom
fix/dia-cross-epoch-pause-invariant
Aug 4, 2026
Merged

hardyjosh merged 3 commits into
mainfrom
fix/dia-cross-epoch-pause-invariant

Conversation

@hardyjosh

@hardyjosh hardyjosh commented Jul 27, 2026 •

Copy link
Copy Markdown
Collaborator

HIGH-severity stale-price risk around corporate actions, reported by a
curator's dev. The share price multiplies a DIA equity price by the vault's
LIVE totalAssets/totalSupply ratio, and both must belong to the same
corporate-action epoch. When an action (e.g. a stock split) 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.

Nothing enforced that the pause outlasts the staleness window. With
pauseTimeAfter < maxAge, once the pause lifts a pre-action DIA price is still
"fresh" (age < maxAge, so accepted) and gets paired with the already-rebalanced
post-action ratio. On a 2:1 split the share reads ~2x its true value → a
borrower can draw against phantom collateral → bad debt. The staleness check
bounds age, not epoch, so it does not close this.

Fix: enforce the invariant pauseTimeAfter >= maxAge at initialize
(PauseTimeAfterBelowMaxAge). This makes the oldest still-fresh push at
pause-lift no older than the action's effectiveTime, so only same-epoch
(post-action) prices are ever served. Documented in the config + contract
NatSpec and the README (with a margin recommendation — the exact boundary is
only safe if DIA never times a push at the precise effectiveTime instant).

  • New PauseTimeAfterBelowMaxAge(pauseTimeAfter, maxAge) error + init guard.
  • Tests: below-boundary reverts, exact-boundary accepted, and the previously
    "valid" pre-window-only config (pauseTimeAfter == 0) now correctly rejected
    as the vulnerable shape. Verified each fails without the guard.
  • Fixed configs that violated the new invariant: the fork test
    (maxAge 3650 days / pauseTimeAfter 1h -> 6h/6h) — the exact fixture the
    reporter flagged — the E2E test (1h -> 2h post-window), and the deployer
    idempotence test (vary pauseTimeAfter, not maxAge).

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_016cakZwdXx1U2yczfFHwVV1

Summary by CodeRabbit

  • New Features

    • Added configuration validation requiring pauseTimeAfter >= maxAge.
    • Updated DIA staleness handling so values exactly maxAge old are treated as stale.
  • Documentation

    • Updated README guidance and the Integrator Quickstart example for the timing invariant.
  • Tests

    • Expanded coverage for invalid configurations, timing boundaries, stale data, and pause handovers.
    • Adjusted end-to-end and fork test timing for deterministic results.

HIGH-severity stale-price risk around corporate actions, reported by a
curator's dev. The share price multiplies a DIA equity price by the vault's
LIVE totalAssets/totalSupply ratio, and both must belong to the same
corporate-action epoch. When an action (e.g. a stock split) 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.

Nothing enforced that the pause outlasts the staleness window. With
pauseTimeAfter < maxAge, once the pause lifts a pre-action DIA price is still
"fresh" (age < maxAge, so accepted) and gets paired with the already-rebalanced
post-action ratio. On a 2:1 split the share reads ~2x its true value → a
borrower can draw against phantom collateral → bad debt. The staleness check
bounds age, not epoch, so it does not close this.

Fix: enforce the invariant `pauseTimeAfter >= maxAge` at initialize
(PauseTimeAfterBelowMaxAge). This makes the oldest still-fresh push at
pause-lift no older than the action's effectiveTime, so only same-epoch
(post-action) prices are ever served. Documented in the config + contract
NatSpec and the README (with a margin recommendation — the exact boundary is
only safe if DIA never times a push at the precise effectiveTime instant).

- New PauseTimeAfterBelowMaxAge(pauseTimeAfter, maxAge) error + init guard.
- Tests: below-boundary reverts, exact-boundary accepted, and the previously
  "valid" pre-window-only config (pauseTimeAfter == 0) now correctly rejected
  as the vulnerable shape. Verified each fails without the guard.
- Fixed configs that violated the new invariant: the fork test
  (maxAge 3650 days / pauseTimeAfter 1h -> 6h/6h) — the exact fixture the
  reporter flagged — the E2E test (1h -> 2h post-window), and the deployer
  idempotence test (vary pauseTimeAfter, not maxAge).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016cakZwdXx1U2yczfFHwVV1
@hardyjosh
hardyjosh marked this pull request as ready for review July 27, 2026 12:44

hardyjosh commented Jul 27, 2026 •

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The DIA vault oracle now rejects configurations where pauseTimeAfter is less than maxAge and treats DIA data exactly maxAge old as stale. Documentation, tests, deployment determinism checks, and integration timing fixtures were updated.

Changes

DIA oracle timing invariant

Layer / File(s) Summary
Initialization and staleness boundary
src/concrete/oracle/DIAVaultOracle.sol
Adds PauseTimeAfterBelowMaxAge, documents the timing invariant, enforces it during initialization, and changes the DIA staleness boundary to age >= maxAge.
Configuration and boundary tests
test/src/concrete/oracle/DIAVaultOracle.t.sol, test/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.t.sol
Tests invalid and equal timing configurations, exact-boundary staleness, freshness inside the boundary, pause handover behavior, fuzzed timing cases, and distinct CREATE2 configurations.
Examples and integration timing fixtures
README.md, test/src/e2e/DIAStackE2E.t.sol, test/src/fork/DIAVaultOracleForkBase.t.sol
Updates timing examples and comments, and seeds fresh DIA data for deterministic fork assertions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Initializer
  participant DIAVaultOracle
  participant DIAOracle
  Initializer->>DIAVaultOracle: initialize(config)
  DIAVaultOracle-->>Initializer: Revert if pauseTimeAfter < maxAge
  DIAVaultOracle->>DIAOracle: Read DIA feed
  DIAVaultOracle-->>Initializer: Revert if age >= maxAge
Loading

Possibly related PRs

Suggested reviewers: juanirios

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely states the main fix: enforcing pauseTimeAfter >= maxAge to prevent cross-epoch mispricing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dia-cross-epoch-pause-invariant

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/concrete/oracle/DIAVaultOracle.sol`:
- Around line 288-301: Prevent the equality boundary from being accepted in the
DIAVaultOracle configuration by requiring pauseTimeAfter to be strictly greater
than maxAge, then update the related NatSpec at
src/concrete/oracle/DIAVaultOracle.sol:153-167 to describe the strict guarantee.
At test/src/concrete/oracle/DIAVaultOracle.t.sol:145-180, reject equality and
cover the exact effectiveTime stale-price case; update README.md:61-73
consistently. Configure PAUSE_AFTER with a margin above MAX_AGE in
test/src/e2e/DIAStackE2E.t.sol:40-41 and
test/src/fork/DIAVaultOracleForkBase.t.sol:48-53.

In `@test/src/fork/DIAVaultOracleForkBase.t.sol`:
- Around line 48-53: Update the fork test configuration around MAX_AGE and
PAUSE_AFTER so it no longer relies on the live DIA COIN value being refreshed
within six hours at the latest fork block. Use a deterministic pinned block or
fixture, seed a mock feed, or explicitly handle stale-data reverts while
preserving the cross-epoch invariant and test coverage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 430603eb-744d-4bff-b39b-5c81afe3f90a

📥 Commits

Reviewing files that changed from the base of the PR and between 229755d and 8a90a93.

📒 Files selected for processing (6)
  • README.md
  • src/concrete/oracle/DIAVaultOracle.sol
  • test/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.t.sol
  • test/src/concrete/oracle/DIAVaultOracle.t.sol
  • test/src/e2e/DIAStackE2E.t.sol
  • test/src/fork/DIAVaultOracleForkBase.t.sol

Comment thread src/concrete/oracle/DIAVaultOracle.sol
Comment thread test/src/fork/DIAVaultOracleForkBase.t.sol Outdated
…live feed

Addresses CodeRabbit on #280.

(1) The equal boundary (pauseTimeAfter == maxAge) previously left a one-instant
hole: with the staleness rule `age > maxAge` accepting age == maxAge, a
pre-action push timestamped exactly at effectiveTime could be served at
pause-lift. Fix: reject the staleness edge (`age >= maxAge` is stale). Now at
pause-lift t = effectiveTime + pauseTimeAfter, any served push is timestamped
> t - maxAge >= effectiveTime — strictly post-action — so pauseTimeAfter >=
maxAge is unconditionally airtight with no same-instant ambiguity. This also
aligns with the fail-closed staleness convention (the edge counts as stale).
Flipped the boundary test (age == maxAge is now stale) + added a just-inside
case; updated NatSpec and README to drop the "only safe if..." caveat.

(2) The fork test's maxAge=6h made the pre-schedule read depend on the LIVE DIA
COIN feed being <6h fresh at the fork block. Mock the DIA feed to a fresh value
for that read (DIA is not the fork test's subject — the real corporate-action
vault is; the pause gate reverts before the DIA read anyway) and drop maxAge to
1h. No live-feed-freshness CI dependency.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016cakZwdXx1U2yczfFHwVV1
hardyjosh pushed a commit that referenced this pull request Jul 30, 2026
…bol desc

Addresses three DIAVaultOracle audit findings on top of the cross-epoch
pause invariant (PR #280):

- #261 (LOW): a future DIA push timestamp underflow-panicked in
  _readDIAChecked. Guard the staleness subtraction with
  `timestamp <= block.timestamp` so a future-timestamped push resolves to
  the fresh (non-stale) branch instead of a bare Panic(0x11). New test
  testLatestAnswerFutureTimestampNotStale.

- #262 (LOW): document the donation / share-price-inflation trust model in
  the contract and _vaultSharePrice NatSpec — the priced vault MUST be an
  ST0x wtStock whose totalAssets() is accounted (not raw balanceOf) so a
  donation cannot inflate the ratio. New test
  testVaultTotalAssetsSourceIsAccounted pinning the invariant.

- #274 (INFO): document the deliberate bare-symbol deviation on the
  description() override and soften the AggregatorV2V3Interface NatSpec so
  the two no longer contradict. New test
  testDescriptionReturnsBareSymbolNotPairString.

- #272 (partial): float IAggregatorV2V3.sol pragma to ^0.8.25 per the
  rainlanguage per-file-kind convention for vendored interfaces.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016cakZwdXx1U2yczfFHwVV1

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/src/concrete/oracle/DIAVaultOracle.t.sol (1)

383-407: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add an end-to-end equality-boundary test.

These tests validate initialization and staleness independently, but do not exercise the composed invariant: a pre-action DIA push timestamped at effectiveTime must not be served when the post-action pause expires at effectiveTime + maxAge. Add a test that drives MockCorporateActions through that boundary and expects stale-price rejection.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/src/concrete/oracle/DIAVaultOracle.t.sol` around lines 383 - 407, Add an
end-to-end equality-boundary test alongside
testLatestAnswerAtMaxAgeBoundaryIsStale and
testLatestAnswerJustInsideMaxAgeNotStale. Drive MockCorporateActions with a
pre-action DIA push timestamped at effectiveTime, advance or warp until the
post-action pause expires at effectiveTime + MAX_AGE, then call the oracle
pricing path and expect DIAPriceStale rejection, preserving the composed
cross-epoch invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/src/concrete/oracle/DIAVaultOracle.t.sol`:
- Around line 383-407: Add an end-to-end equality-boundary test alongside
testLatestAnswerAtMaxAgeBoundaryIsStale and
testLatestAnswerJustInsideMaxAgeNotStale. Drive MockCorporateActions with a
pre-action DIA push timestamped at effectiveTime, advance or warp until the
post-action pause expires at effectiveTime + MAX_AGE, then call the oracle
pricing path and expect DIAPriceStale rejection, preserving the composed
cross-epoch invariant.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 053a30f2-2804-4d7a-8c1a-923887a4d835

📥 Commits

Reviewing files that changed from the base of the PR and between 8a90a93 and 5faf379.

📒 Files selected for processing (4)
  • README.md
  • src/concrete/oracle/DIAVaultOracle.sol
  • test/src/concrete/oracle/DIAVaultOracle.t.sol
  • test/src/fork/DIAVaultOracleForkBase.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md

Addresses the outstanding CodeRabbit nitpick on this PR: the pause and
staleness guards were each pinned in isolation, but nothing exercised the
composed invariant they exist to buy — that a pre-action DIA push is never
served once the post-action pause lifts.

Adds two tests:

- testPreActionPriceNeverServedAcrossPauseHandover — drives the real
  MockCorporateActions through the handover instant and pins that the
  windows abut. This is currently the ONLY test in the suite that detects
  narrowing the inclusive post-pause boundary (<= to <); that edge was
  previously unpinned.

- testFuzzPreActionPriceNeverServed — the invariant in its strongest form:
  for any config initialize() accepts, and at any instant from the action
  onward, a push timestamped at or before effectiveTime must revert rather
  than be served, and for one of the two intended reasons. Bounds are kept
  tight around the pause-lift instant deliberately: the property can only
  break where the two windows meet, and a wide sweep dilutes the runs that
  matter — with wide bounds the fuzzer did not find the corner at all.

Both were mutation-verified: widening the staleness edge (>= to >) together
with narrowing the pause window opens a real servable instant, and the fuzz
test finds that counterexample in ~100 runs. Test-only change; src/ is
untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XJKzQfAVLi52RPb3nGPhwF
hardyjosh pushed a commit that referenced this pull request Aug 4, 2026
…bol desc

Addresses three DIAVaultOracle audit findings on top of the cross-epoch
pause invariant (PR #280):

- #261 (LOW): a future DIA push timestamp underflow-panicked in
  _readDIAChecked. Guard the staleness subtraction with
  `timestamp <= block.timestamp` so a future-timestamped push resolves to
  the fresh (non-stale) branch instead of a bare Panic(0x11). New test
  testLatestAnswerFutureTimestampNotStale.

- #262 (LOW): document the donation / share-price-inflation trust model in
  the contract and _vaultSharePrice NatSpec — the priced vault MUST be an
  ST0x wtStock whose totalAssets() is accounted (not raw balanceOf) so a
  donation cannot inflate the ratio. New test
  testVaultTotalAssetsSourceIsAccounted pinning the invariant.

- #274 (INFO): document the deliberate bare-symbol deviation on the
  description() override and soften the AggregatorV2V3Interface NatSpec so
  the two no longer contradict. New test
  testDescriptionReturnsBareSymbolNotPairString.

- #272 (partial): float IAggregatorV2V3.sol pragma to ^0.8.25 per the
  rainlanguage per-file-kind convention for vendored interfaces.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016cakZwdXx1U2yczfFHwVV1

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/src/concrete/oracle/DIAVaultOracle.t.sol (1)

515-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a positive control inside the fuzz body.

The fuzz assertions only require a revert. A mutant that makes latestAnswer revert on every input passes this test. The concrete test at Lines 455-459 is the only positive control, and it pins one config. Add a post-action push at the same instant when the read is not paused, then assert the oracle serves it.

♻️ Optional strengthening of the fuzz assertions
     bytes4 reason = bytes4(ret);
     assertTrue(
         reason == OraclePausedCorporateAction.selector || reason == DIAPriceStale.selector,
         "must revert paused or stale, not incidentally"
     );
+
+    // Positive control: once the pause lifts, a strictly post-action push at
+    // the same instant must be served, so a mutant that reverts on every
+    // input cannot satisfy this test.
+    if (reason == DIAPriceStale.selector) {
+        diaOracle.setValue(SYMBOL, 50e18, uint128(block.timestamp));
+        assertEq(oracle.latestAnswer(), int256(50e8), "post-action push must price once unpaused");
+    }

As per coding guidelines "Run forge test to execute all unit and fuzz tests".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/src/concrete/oracle/DIAVaultOracle.t.sol` around lines 515 - 525,
Strengthen the fuzz test around DIAVaultOracle.latestAnswer by adding a
post-action DIA price push at the same effective post-action instant when the
oracle is not paused, then call latestAnswer and assert it successfully serves
the newly pushed value. Keep the existing pre-action rejection and
reason-selector assertions, and ensure the positive control proves the oracle is
not simply reverting for every input.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/src/concrete/oracle/DIAVaultOracle.t.sol`:
- Around line 515-525: Strengthen the fuzz test around
DIAVaultOracle.latestAnswer by adding a post-action DIA price push at the same
effective post-action instant when the oracle is not paused, then call
latestAnswer and assert it successfully serves the newly pushed value. Keep the
existing pre-action rejection and reason-selector assertions, and ensure the
positive control proves the oracle is not simply reverting for every input.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e8508489-998c-4b3a-9044-03b7749e59fa

📥 Commits

Reviewing files that changed from the base of the PR and between 5faf379 and ffe1e03.

📒 Files selected for processing (1)
  • test/src/concrete/oracle/DIAVaultOracle.t.sol

@hardyjosh
hardyjosh merged commit 4172483 into main Aug 4, 2026
9 checks passed
hardyjosh pushed a commit that referenced this pull request Aug 4, 2026
…bol desc

Addresses three DIAVaultOracle audit findings on top of the cross-epoch
pause invariant (PR #280):

- #261 (LOW): a future DIA push timestamp underflow-panicked in
  _readDIAChecked. Guard the staleness subtraction with
  `timestamp <= block.timestamp` so a future-timestamped push resolves to
  the fresh (non-stale) branch instead of a bare Panic(0x11). New test
  testLatestAnswerFutureTimestampNotStale.

- #262 (LOW): document the donation / share-price-inflation trust model in
  the contract and _vaultSharePrice NatSpec — the priced vault MUST be an
  ST0x wtStock whose totalAssets() is accounted (not raw balanceOf) so a
  donation cannot inflate the ratio. New test
  testVaultTotalAssetsSourceIsAccounted pinning the invariant.

- #274 (INFO): document the deliberate bare-symbol deviation on the
  description() override and soften the AggregatorV2V3Interface NatSpec so
  the two no longer contradict. New test
  testDescriptionReturnsBareSymbolNotPairString.

- #272 (partial): float IAggregatorV2V3.sol pragma to ^0.8.25 per the
  rainlanguage per-file-kind convention for vendored interfaces.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016cakZwdXx1U2yczfFHwVV1
hardyjosh pushed a commit that referenced this pull request Aug 4, 2026
…bol desc

Addresses three DIAVaultOracle audit findings on top of the cross-epoch
pause invariant (PR #280):

- #261 (LOW): a future DIA push timestamp underflow-panicked in
  _readDIAChecked. Guard the staleness subtraction with
  `timestamp <= block.timestamp` so a future-timestamped push resolves to
  the fresh (non-stale) branch instead of a bare Panic(0x11). New test
  testLatestAnswerFutureTimestampNotStale.

- #262 (LOW): document the donation / share-price-inflation trust model in
  the contract and _vaultSharePrice NatSpec — the priced vault MUST be an
  ST0x wtStock whose totalAssets() is accounted (not raw balanceOf) so a
  donation cannot inflate the ratio. New test
  testVaultTotalAssetsSourceIsAccounted pinning the invariant.

- #274 (INFO): document the deliberate bare-symbol deviation on the
  description() override and soften the AggregatorV2V3Interface NatSpec so
  the two no longer contradict. New test
  testDescriptionReturnsBareSymbolNotPairString.

- #272 (partial): float IAggregatorV2V3.sol pragma to ^0.8.25 per the
  rainlanguage per-file-kind convention for vendored interfaces.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016cakZwdXx1U2yczfFHwVV1
hardyjosh pushed a commit that referenced this pull request Aug 9, 2026
…bol desc

Addresses four DIAVaultOracle audit findings on top of the cross-epoch
pause invariant (PR #280):

- #261 (LOW): a future DIA push timestamp underflow-panicked in
  _readDIAChecked. Guard the staleness subtraction with
  `timestamp <= block.timestamp` so a future-timestamped push resolves to
  the fresh (non-stale) branch instead of a bare Panic(0x11). New test
  testLatestAnswerFutureTimestampNotStale.

- #262 (LOW): document the donation / share-price trust model truthfully.
  The finding asked for the analysis; this is the analysis, and the
  conclusion is that no mitigation is warranted.

  The production wtStock (StoxWrappedTokenVault) overrides only
  name()/symbol() and inherits OZ ERC-4626's raw-balanceOf totalAssets(),
  so donations DO move the share ratio, and the upstream authorizer
  permits any TRANSFER_SHARES while certification is valid — the transfer
  path is permissionless, not KYC-gated. Both facts were previously
  documented backwards.

  That is not a manipulation surface. A donation adds real assets the
  shares genuinely redeem for (no phantom collateral), mints the donor
  nothing, and cannot be withdrawn — so it is value-additive and
  negative-EV for the donor: cost D buys at most LTV * yourShareOfSupply
  * D of borrowing power. The classic ERC-4626 donation attack is a
  ROUNDING attack on depositors, lives in the vault's share accounting,
  and is not addressable from an oracle.

  Deliberately NOT mitigated: no sanity band, drift limit or ratio anchor
  gates reads. Such a gate fails closed on a legitimate NAV move (a large
  dividend or redemption), and an oracle that stops answering is worse for
  a lending market than one that answers correctly — liquidations halt
  while positions keep moving, accruing bad debt. The real stale-ratio
  risk is the corporate-action rebalance, already handled by the mandatory
  auto-pause. New test testDonationMovesRatioAndIsServed pins the decided
  behaviour: the donation is priced through, so any band added to the read
  path fails the test.

- #274 (INFO): document the deliberate bare-symbol deviation on the
  description() override and soften the AggregatorV2V3Interface NatSpec so
  the two no longer contradict. New test
  testDescriptionReturnsBareSymbolNotPairString.

- #272 (partial): float IAggregatorV2V3.sol pragma to ^0.8.25 per the
  rainlanguage per-file-kind convention for vendored interfaces.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hi8wq9YkjWPcGACXCEKeL
hardyjosh pushed a commit that referenced this pull request Aug 9, 2026
…bol desc

Addresses four DIAVaultOracle audit findings on top of the cross-epoch
pause invariant (PR #280):

- #261 (LOW): a future DIA push timestamp underflow-panicked in
  _readDIAChecked. Guard the staleness subtraction with
  `timestamp <= block.timestamp` so a future-timestamped push resolves to
  the fresh (non-stale) branch instead of a bare Panic(0x11). New test
  testLatestAnswerFutureTimestampNotStale.

- #262 (LOW): document the donation / share-price trust model truthfully.
  The finding asked for the analysis; this is the analysis, and the
  conclusion is that no mitigation is warranted.

  The production wtStock (StoxWrappedTokenVault) overrides only
  name()/symbol() and inherits OZ ERC-4626's raw-balanceOf totalAssets(),
  so donations DO move the share ratio, and the upstream authorizer
  permits any TRANSFER_SHARES while certification is valid — the transfer
  path is permissionless, not KYC-gated. Both facts were previously
  documented backwards.

  That is not a manipulation surface. A donation adds real assets the
  shares genuinely redeem for (no phantom collateral), mints the donor
  nothing, and cannot be withdrawn — so it is value-additive and
  negative-EV for the donor: cost D buys at most LTV * yourShareOfSupply
  * D of borrowing power. The classic ERC-4626 donation attack is a
  ROUNDING attack on depositors, lives in the vault's share accounting,
  and is not addressable from an oracle.

  Deliberately NOT mitigated: no sanity band, drift limit or ratio anchor
  gates reads. Such a gate fails closed on a legitimate NAV move (a large
  dividend or redemption), and an oracle that stops answering is worse for
  a lending market than one that answers correctly — liquidations halt
  while positions keep moving, accruing bad debt. The real stale-ratio
  risk is the corporate-action rebalance, already handled by the mandatory
  auto-pause. New test testDonationMovesRatioAndIsServed pins the decided
  behaviour: the donation is priced through, so any band added to the read
  path fails the test.

- #274 (INFO): document the deliberate bare-symbol deviation on the
  description() override and soften the AggregatorV2V3Interface NatSpec so
  the two no longer contradict. New test
  testDescriptionReturnsBareSymbolNotPairString.

- #272 (partial): float IAggregatorV2V3.sol pragma to ^0.8.25 per the
  rainlanguage per-file-kind convention for vendored interfaces.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hi8wq9YkjWPcGACXCEKeL
hardyjosh added a commit that referenced this pull request Aug 10, 2026
…287)

Deploy/deployers audit group. **Stacked on #279 → #280** (edits `Deploy.sol`/`Deploy.t.sol` and the fork/deployer tests those two change). Closes #256, #267, #268, #258, #260, #214.

- **#256 (MEDIUM)** — `run()` suite-dispatch coverage: both success branches + the unknown-suite revert.
- **#267 (LOW)** — real external assertions on the signed-price deploy test (signer, timeout, `ORACLE_ADMIN_ROLE`, both beacon owners, adapter→central binding) via return values.
- **#268 (LOW)** — rename all four deployer immutables to `i`+UpperCamel (`iDIAVaultOracleBeacon`, `iST0xPriceOracleBeacon`, `iMorphoPairAdapterBeacon`, `iCentral`) across deployers, getters, `Deploy.sol`, and every deployer/salt-derivation test.
- **#258 (MEDIUM)** — fork test imports `LibProdTokensBase.COIN_*` from pinned st0x-deploy; `DIA_FEED` centralised in a new `src/lib/LibDIAFeed.sol`.
- **#260 (MEDIUM)** — no-RPC fork path `require()`s `FORK_TESTS==0`; `fork-tests.yaml` sets `FORK_TESTS=1` so a missing RPC in the authoritative job fails loudly.
- **#214 (LOW)** — grep-based `suite-name-sync` CI job asserting the workflow dropdown options and `Deploy.sol`'s `keccak256(...)` suite preimages are exactly equal (offline, no `fs_permissions` needed). Verified it fails on a one-char typo.

143 tests passing (stable ×3, incl. live fork); slither clean; single-contract + reuse green. Env-touching tests consolidated to avoid the process-global `vm.setEnv` race.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->

## Summary by CodeRabbit

- **New Features**
  - Added Base network DIA feed configuration for deployment and fork testing.
  - Deployment tooling now exposes deployed oracle and adapter contracts for verification.
  - Added automated checks to keep deployment suite names synchronized.

- **Improvements**
  - Fork tests now use the standard Base RPC configuration with failover support.
  - Updated deployment interfaces with clearer, consistent accessor naming.
  - Improved deployment validation for environment configuration, contract wiring, and key separation.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
hardyjosh added a commit that referenced this pull request Aug 10, 2026
Audit-fix group for the DIA oracle. Closes #261, #262, #274; partially addresses #272.

Rebased onto `main` now that [#280](https://app.graphite.com/github/pr/ST0x-Technology/st0x.oracle/280) (the cross-epoch invariant fix) has merged. Single commit.

- **#261 (LOW)** — guard the staleness subtraction so a future-dated DIA push resolves to the fresh branch instead of an underflow `Panic(0x11)`.
- **#262 (LOW)** — document the donation / share-price trust model. **The finding asked for the analysis; the analysis concludes no mitigation is warranted.** See below.
- **#274 (INFO)** — document the deliberate bare-symbol `description()` deviation and soften the interface NatSpec so they no longer contradict.
- **#272 (part, LOW)** — float `IAggregatorV2V3.sol` pragma to `^0.8.25`.

### On #262 — what changed and why

An earlier revision of this PR claimed the priced vault's `totalAssets()` was an *accounted* quantity that donations could not move, and pinned that with a test. **Both were false assurance**, and a later revision over-corrected by adding a drift-band mitigation. Neither survives; here is the corrected position.

**The facts, verified against source:**

- `StoxWrappedTokenVault` (st0x.deploy) overrides only `name()` and `symbol()`. It inherits OpenZeppelin ERC-4626's default `totalAssets()`, which is raw `IERC20(asset()).balanceOf(address(this))`. Donations **do** move the share ratio.
- The transfer path is **permissionless**, not KYC-gated. `OffchainAssetReceiptVaultAuthorizerV1.authorize()` returns unconditionally for `TRANSFER_SHARES` while certification is valid — the only gate is a system-wide freeze on certification lapse.

**Why that is not a manipulation surface:**

A donation adds real assets that the shares genuinely redeem for, so there is no phantom collateral — a borrower's wtStock really is worth more. A bare ERC-20 transfer mints the donor nothing and cannot be withdrawn, so the value spreads pro-rata across existing holders. Donating `D` buys at most `LTV × yourShareOfSupply × D` of extra borrowing power at a cost of `D` — strictly negative-EV unless you already own essentially the whole supply, at which point it is circular.

The classic ERC-4626 donation attack is a **rounding** attack on depositors (a near-empty vault plus a donation makes the next depositor's shares round to zero). It lives in the vault's own share accounting, harms depositors rather than price consumers, and is not addressable from an oracle.

**Deliberately not mitigated.** No sanity band, drift limit or ratio anchor gates reads. Such a gate fails closed on a legitimate NAV move (a large dividend or redemption), and an oracle that stops answering is worse for a lending market than one that answers correctly — liquidations halt while positions keep moving, which accrues bad debt. The real stale-ratio risk is the corporate-action rebalance, and that is already handled by the mandatory auto-pause.

`testDonationMovesRatioAndIsServed` pins this: a donation is priced straight through at the true new ratio, so any band added to the read path fails the test.

### Verification

167 tests passing (including the fork test against live Base); `forge fmt --check`, `slither` (0 findings), `rainix-sol-single-contract` and `reuse lint` all green locally before push.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
hardyjosh pushed a commit that referenced this pull request Aug 10, 2026
…timestamps

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hi8wq9YkjWPcGACXCEKeL
hardyjosh added a commit that referenced this pull request Aug 10, 2026
…timestamps (#292)

**C1 (HIGH)** — the cross-epoch invariant #280 called "airtight" at `pauseTimeAfter == maxAge` is defeated by DIA feed forward clock skew. Staleness ages a push by its own SOURCE timestamp while 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 pause-lift 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't deploy, and the NatSpec states operators must size the margin above the feed's worst-case forward skew (prod's 1h margin is the reference) rather than claiming equality is airtight. `testForwardSkewIsRejectedWithinMargin` pins the exact tolerance boundary *and* the operator-owned residual beyond it.

Also on this file:
- **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. Now clamped to `block.timestamp` (fresh future push reports age 0); `roundId` still tracks the raw timestamp.
- **C2/C4/C5 (doc)** — corrected NatSpec the code contradicted: `roundId` is a freshness token, not monotonic; a uint256-range overflow surfaces as `FixedDecimalOverflow`, not `VaultSharePriceOverflow`; a non-zero ERC-4626 decimals offset is *mispriced*, not merely un-pausable.

`script/` untouched. Stacked on [#291](https://app.graphite.com/github/pr/ST0x-Technology/st0x.oracle/291). 191 tests; fmt/slither/reuse/single-contract clean. The margin-enforcement approach (strict `>` + operator-sized margin, no baked-in skew constant) was your call.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant