Repository navigation
audit: DIAVaultOracle correctness + docs (#261, #262, #274, #272) - #284
Conversation
📝 WalkthroughWalkthroughThe PR documents raw-balance vault pricing and bare DIA symbols, changes future DIA timestamps to be treated as fresh, and adds tests for descriptions, donation-driven repricing, and future timestamps. ChangesOracle edge-case behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
502e9ee to
a081a68
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ffe1e03 to
4172483
Compare
a081a68 to
d32d908
Compare
d32d908 to
4a11acf
Compare
|
@coderabbitai review |
|
|
c61b53b replaces the #262 remediation: the previous fix took option (a) (assert totalAssets is accounted), but the production StoxWrappedTokenVault inherits OZ's raw-balanceOf This commit takes option (b):
Local gates: forge fmt / 178 tests / slither / reuse / single-contract all green; fork suite passes on a public Base RPC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/concrete/oracle/DIAVaultOracle.sol (2)
608-647: 🚀 Performance & Scalability | 🔵 TrivialNote the added external-call cost on the price read path.
_validateRatioAnchoredadds acompletedActionCount()call to everylatestAnswerandlatestRoundData. Combined withinPauseWindow,totalAssets(),totalSupply(), and the DIAgetValue, each price read now performs several external calls. Lending markets call these functions inside liquidation and borrow paths, where gas is on the critical path.The logic is correct and fails closed on every branch. This is a cost observation, not a defect. If gas becomes a concern, one option is to fold the completed-action read into the existing
inPauseWindowprobe so the corporate-actions contract is called once per read.🤖 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 `@src/concrete/oracle/DIAVaultOracle.sol` around lines 608 - 647, The added completedActionCount() external call in _validateRatioAnchored increases gas on every price read; if optimizing this path, fold the completed-action read into the existing inPauseWindow probe so the corporate-actions contract is queried only once per latestAnswer/latestRoundData execution, while preserving the current validation behavior.
543-561: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider bounding how far in the future a DIA timestamp may be.
The guard correctly removes the
Panic(0x11)underflow for future-dated pushes. That matches issue#261. One consequence is worth an explicit decision: a push timestamped arbitrarily far in the future is treated as fresh forever, so themaxAgestaleness check no longer constrains that feed at all. A single bad push with a corrupt future timestamp would freeze the served price with no fail-closed path.If DIA feeds are trusted to never push a materially future timestamp, keep the current behavior and record that assumption in the comment. If not, accept only a small clock-skew tolerance and treat anything beyond it as invalid.
♻️ Optional: bounded future tolerance
- if (uint256(timestamp) <= block.timestamp && block.timestamp - uint256(timestamp) >= $.maxAge) { + if (uint256(timestamp) > block.timestamp) { + // Tolerate only small feed/chain clock skew ahead of `now`. + if (uint256(timestamp) - block.timestamp > MAX_FUTURE_SKEW) { + revert DIAPriceStale(uint256(timestamp)); + } + } else if (block.timestamp - uint256(timestamp) >= $.maxAge) { revert DIAPriceStale(uint256(timestamp)); }🤖 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 `@src/concrete/oracle/DIAVaultOracle.sol` around lines 543 - 561, Decide the intended future-timestamp policy in the staleness guard: either document in its comment that DIA timestamps are trusted not to be materially future-dated, or add a small clock-skew bound and reject timestamps beyond it as invalid. Update the logic around the existing timestamp comparison while preserving the underflow protection and fail-closed stale behavior.test/src/e2e/DIAStackE2E.t.sol (2)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the
internalvisibility keyword for consistency.The neighbouring constants at lines 38-42 all declare
internal. Behavior is identical, because Solidity defaults constants tointernal.♻️ Proposed change
- uint32 constant DRIFT_BPS_PER_DAY = 100; + uint32 internal constant DRIFT_BPS_PER_DAY = 100;🤖 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/e2e/DIAStackE2E.t.sol` at line 43, Update the DRIFT_BPS_PER_DAY constant declaration to explicitly include internal visibility, matching the neighboring constants while preserving its existing value and type.
175-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdvance the completed-action count at completion time.
Line 180 advances the mock count after the post-window warp. Line 170 is where the mock transitions the action to completed. On the real vault the count advances at completion, so the mock state at lines 170-173 is inconsistent for one step. The assertions still hold, because the pause gate rejects the read at line 172 before the anchor check runs. Moving the call to line 170 models the real lifecycle and keeps the pause-then-unanchored ordering explicit.
♻️ Proposed change
corporateActions.setEarliestPending(NODE_NONE, 0, 0); corporateActions.setLatestCompleted(1, type(uint256).max, effectiveTime); + corporateActions.setCompletedActionCount(1); vm.expectRevert(abi.encodeWithSelector(OraclePausedCorporateAction.selector, effectiveTime)); oracle.latestAnswer(); @@ vm.warp(uint256(effectiveTime) + uint256(PAUSE_AFTER) + 1); _freshDIAAt100(); - corporateActions.setCompletedActionCount(1); vm.expectRevert(abi.encodeWithSelector(VaultRatioNotAnchored.selector));🤖 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/e2e/DIAStackE2E.t.sol` around lines 175 - 184, Move corporateActions.setCompletedActionCount(1) from the post-window section to the action-completion transition around the existing completion logic near lines 170–173. Keep the post-window warp, _freshDIAAt100(), VaultRatioNotAnchored expectation, and checkpointRatio() flow unchanged so the mock advances its completed count at completion while preserving pause-then-unanchored assertions.test/mocks/MockCorporateActions.sol (1)
82-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the state variable to the declaration block and name the setter parameter.
_completedActionCountsits between function definitions, while_earliestPendingand_latestCompletedare declared at lines 35-36. Solidity style places state variables before functions. The parametervalso differs from the descriptive parameter names used by the other setters.♻️ Proposed layout and naming change
- uint256 private _completedActionCount; - - function setCompletedActionCount(uint256 v) external { - _completedActionCount = v; - } - + function setCompletedActionCount(uint256 count) external { + _completedActionCount = count; + } + function completedActionCount() external view override returns (uint256) { return _completedActionCount; }Declare the variable next to the other stub state:
StubAction private _earliestPending; StubAction private _latestCompleted; uint256 private _completedActionCount;🤖 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/mocks/MockCorporateActions.sol` around lines 82 - 90, Move _completedActionCount to the state-variable declaration block beside _earliestPending and _latestCompleted, removing its current declaration between functions. Rename the setCompletedActionCount parameter from v to a descriptive name consistent with the other setters, and use that parameter in the assignment.README.md (1)
100-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the deploy-time anchoring precondition.
The note states that the oracle anchors the ratio at deploy.
initializeonly anchors when the vault is already seeded and no pause window is open. If an integrator mints an oracle against an unseeded vault, or inside a corporate-action pause window, no anchor is recorded and every read revertsVaultRatioNotAnchoreduntil someone callscheckpointRatio(). The unit tests pin both cases (testVaultRatioBandUnseededInitFailsClosedUntilCheckpoint,testVaultRatioBandInitDuringPauseLeavesUnanchored). Add one sentence so integrators do not deploy into a silently unservable state.📝 Proposed wording
> anchors the ratio at deploy (and at each `checkpointRatio()`) and rejects > reads whose live ratio drifts outside `anchor * (1 ± bps/day accrued)` between > corporate actions (`VaultRatioOutOfBand`, fail closed). A completed corporate +> action (split, dividend) stops serving until a permissionless +> `checkpointRatio()` re-anchors on the post-action ratio +> (`VaultRatioNotAnchored`) — call it right after the pause window lifts. +> Deploying against an unseeded vault, or inside a pause window, records no +> anchor at all: reads revert `VaultRatioNotAnchored` until the first +> `checkpointRatio()`. Seed the vault before minting the oracle, or checkpoint +> immediately after. -> action (split, dividend) stops serving until a permissionless -> `checkpointRatio()` re-anchors on the post-action ratio -> (`VaultRatioNotAnchored`) — call it right after the pause window lifts. > Scheduled NAV events all arrive as corporate actions, so the band only needs🤖 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 `@README.md` around lines 100 - 113, The README section describing Vault-ratio drift must add one sentence documenting that initialize does not record an anchor for an unseeded vault or while a corporate-action pause window is open, so reads remain unavailable until permissionless checkpointRatio() is called.test/src/concrete/oracle/DIAVaultOracle.t.sol (1)
388-585: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a fuzz test for the drift band.
The band tests pin concrete points: same-block, 1 day, 2 days, and zero drift. The band itself is a function of arbitrary inputs (
maxRatioDriftPerDayBps, elapsed seconds, live ratio). A fuzz test would pin the property directly: a ratio insideanchor * (1 ± bps * elapsed / 1 days / 10000)serves, and a ratio outside revertsVaultRatioOutOfBand. This also guards the float rounding at the band edges, which the concrete cases do not reach.🧪 Sketch of the property test
/// `@notice` For any accepted drift config, elapsed time, and live ratio, the /// read serves exactly when the ratio sits inside the accrued band. function testFuzzVaultRatioBand(uint32 driftBps, uint64 elapsed, uint256 assets) external { driftBps = uint32(bound(driftBps, 0, 10_000)); elapsed = uint64(bound(elapsed, 0, 30 days)); assets = bound(assets, 0.5e18, 2e18); DIAVaultOracleConfig memory config = _defaultConfig(); config.maxRatioDriftPerDayBps = driftBps; DIAVaultOracle oracle = _deployProxy(config); vm.warp(block.timestamp + elapsed); diaOracle.setValue(SYMBOL, 100e18, uint128(block.timestamp)); vault.setTotalAssets(assets); // Anchor is 1e18/1e18 == 1. Band half-width in 1e18 space: uint256 halfWidth = (uint256(driftBps) * elapsed * 1e18) / (10_000 * 1 days); bool inBand = assets <= 1e18 + halfWidth && assets + halfWidth >= 1e18; (bool served,) = address(oracle).staticcall(abi.encodeCall(DIAVaultOracle.latestAnswer, ())); assertEq(served, inBand, "serves exactly inside the accrued band"); }Note the boundary itself is inclusive in
_validateRatioInBand(gt/lt), so the sketch treats the edge as in-band.As per coding guidelines: "Every feature must have tests, with no exceptions; use fuzz tests wherever appropriate, especially for functions accepting arbitrary inputs."
🤖 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 388 - 585, Add a fuzz test for the vault ratio drift band, alongside the existing testVaultRatioBand* cases, varying maxRatioDriftPerDayBps, elapsed time, and vault assets within valid bounds. Compute the accrued symmetric band using the same fixed-point arithmetic as _validateRatioInBand, then assert latestAnswer succeeds exactly for inclusive in-band ratios and reverts with VaultRatioOutOfBand for out-of-band ratios, including boundary rounding behavior.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.
Inline comments:
In `@src/concrete/oracle/DIAVaultOracle.sol`:
- Around line 624-640: Add a fuzz test alongside the existing ratio-band
scenario tests, such as testVaultRatioBandDriftWithinBandServed, covering
arbitrary uint32 maxRatioDriftPerDayBps values including zero and the maximum,
elapsed values that make drift exceed one, and live ratios exactly on both
calculated upper and lower boundaries. Assert the expected inclusive boundary
behavior and verify that when the lower bound becomes non-positive only the
upper bound constrains validation.
- Around line 580-606: Update checkpointRatio so repeated permissionless
checkpoints within the same between-action period cannot compound the configured
drift allowance: enforce a minimum elapsed interval between accepted same-action
checkpoints, or maintain a corporate-action-reset reference ratio and validate
new ratios against it as well. Preserve immediate acceptance when the completed
action count changes or the anchor is unset, and update the relevant anchor
state consistently.
---
Nitpick comments:
In `@README.md`:
- Around line 100-113: The README section describing Vault-ratio drift must add
one sentence documenting that initialize does not record an anchor for an
unseeded vault or while a corporate-action pause window is open, so reads remain
unavailable until permissionless checkpointRatio() is called.
In `@src/concrete/oracle/DIAVaultOracle.sol`:
- Around line 608-647: The added completedActionCount() external call in
_validateRatioAnchored increases gas on every price read; if optimizing this
path, fold the completed-action read into the existing inPauseWindow probe so
the corporate-actions contract is queried only once per
latestAnswer/latestRoundData execution, while preserving the current validation
behavior.
- Around line 543-561: Decide the intended future-timestamp policy in the
staleness guard: either document in its comment that DIA timestamps are trusted
not to be materially future-dated, or add a small clock-skew bound and reject
timestamps beyond it as invalid. Update the logic around the existing timestamp
comparison while preserving the underflow protection and fail-closed stale
behavior.
In `@test/mocks/MockCorporateActions.sol`:
- Around line 82-90: Move _completedActionCount to the state-variable
declaration block beside _earliestPending and _latestCompleted, removing its
current declaration between functions. Rename the setCompletedActionCount
parameter from v to a descriptive name consistent with the other setters, and
use that parameter in the assignment.
In `@test/src/concrete/oracle/DIAVaultOracle.t.sol`:
- Around line 388-585: Add a fuzz test for the vault ratio drift band, alongside
the existing testVaultRatioBand* cases, varying maxRatioDriftPerDayBps, elapsed
time, and vault assets within valid bounds. Compute the accrued symmetric band
using the same fixed-point arithmetic as _validateRatioInBand, then assert
latestAnswer succeeds exactly for inclusive in-band ratios and reverts with
VaultRatioOutOfBand for out-of-band ratios, including boundary rounding
behavior.
In `@test/src/e2e/DIAStackE2E.t.sol`:
- Line 43: Update the DRIFT_BPS_PER_DAY constant declaration to explicitly
include internal visibility, matching the neighboring constants while preserving
its existing value and type.
- Around line 175-184: Move corporateActions.setCompletedActionCount(1) from the
post-window section to the action-completion transition around the existing
completion logic near lines 170–173. Keep the post-window warp,
_freshDIAAt100(), VaultRatioNotAnchored expectation, and checkpointRatio() flow
unchanged so the mock advances its completed count at completion while
preserving pause-then-unanchored assertions.
🪄 Autofix
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: f92f1dab-f26a-4b91-bfff-58aca2d19965
📒 Files selected for processing (9)
README.mdsrc/concrete/oracle/DIAVaultOracle.solsrc/interface/IAggregatorV2V3.soltest/mocks/MockCorporateActions.soltest/src/concrete/deploy/DIADeployerSaltDerivation.t.soltest/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.t.soltest/src/concrete/oracle/DIAVaultOracle.t.soltest/src/e2e/DIAStackE2E.t.soltest/src/fork/DIAVaultOracleForkBase.t.sol
| function checkpointRatio() external { | ||
| _validateNotPaused(); | ||
| MainStorage storage $ = _main(); | ||
| IERC4626 vaultContract = IERC4626($.vault); | ||
| uint256 totalAssets = vaultContract.totalAssets(); | ||
| uint256 totalSupply = vaultContract.totalSupply(); | ||
| if (totalSupply == 0) revert ZeroVaultSupply(); | ||
| if (totalAssets == 0) revert ZeroVaultAssets(); | ||
|
|
||
| Float ratio = LibDecimalFloat.div( | ||
| LibDecimalFloat.fromFixedDecimalLosslessPacked(totalAssets, 0), | ||
| LibDecimalFloat.fromFixedDecimalLosslessPacked(totalSupply, 0) | ||
| ); | ||
| uint256 completed = ICorporateActionsV1($.corporateActionsVault).completedActionCount(); | ||
|
|
||
| // Between actions the anchor may only move within the band; across a | ||
| // completed action (or from an unset anchor) the live ratio is | ||
| // accepted outright — see the function NatSpec for why. | ||
| if ($.anchorTime != 0 && completed == $.anchorCompletedActionCount) { | ||
| _validateRatioInBand($, ratio); | ||
| } | ||
|
|
||
| $.anchorRatio = ratio; | ||
| $.anchorTime = uint64(block.timestamp); | ||
| $.anchorCompletedActionCount = completed; | ||
| emit VaultRatioCheckpointed(msg.sender, ratio, completed); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any test pins the long-horizon drift budget or repeated checkpoint behavior.
set -euo pipefail
fd -e sol -p 'DIAVaultOracle' test | while IFS= read -r f; do
rg -n -C6 'checkpointRatio' "$f"
done
rg -nP -C4 'maxRatioDriftPerDayBps' --glob 'test/**/*.sol'Repository: ST0x-Technology/st0x.oracle
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "AGENTS.md present:"
test -f AGENTS.md && echo yes || echo no
echo "Locate DIAVaultOracle.sol:"
fd -a 'DIAVaultOracle.sol' . | sed 's#^\./##'
echo "Relevant source excerpts:"
f="$(fd 'DIAVaultOracle.sol' . | head -n1)"
if [ -n "$f" ]; then
wc -l "$f"
sed -n '1,120p' "$f"
sed -n '180,220p' "$f"
sed -n '540,615p' "$f"
sed -n '616,660p' "$f"
fi
echo "Search checkpointRatio and drift references:"
rg -n 'checkpointRatio|maxRatioDriftPerDayBps|anchorRatio|anchorTime|_validateRatioInBand|ratioDrift' . | head -n 200Repository: ST0x-Technology/st0x.oracle
Length of output: 23702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="$(fd 'DIAVaultOracle.t.sol' . | head -n1)"
echo "Test file: $f"
wc -l "$f"
sed -n '460,560p' "$f"
sed -n '1,80p' "$f"
echo "Check tests around checkpoint drift band validation:"
sed -n '475,525p' "$f"
echo "Read-only deterministic drift model for the described path:"
python3 - <<'PY'
def anchored_allowance(bps_per_day, elapsed_days, anchor=1):
return anchor * (1 + bps_per_day / 10000 * elapsed_days)
def reanchored_allowance(bps_per_day, elapsed_days, anchor=1, steps_per_day=1):
rate = 1 + bps_per_day / 10000 / steps_per_day
anchors = int(elapsed_days * steps_per_day)
return anchor * (rate ** anchors)
for bps in [10, 100]:
for years in [1, 3]:
days = int(years * 365)
print(f"{bps} bps, {days} days:")
print(" held overnight:", anchored_allowance(bps, days))
print(" daily checkpoints:", reanchored_allowance(bps, days, steps_per_day=1))
print(" hourly checkpoints:", reanchored_allowance(bps, days, steps_per_day=24))
PYRepository: ST0x-Technology/st0x.oracle
Length of output: 11662
Stop between-action checkpoints from compounding the drift allowance.
checkpointRatio is permissionless and resets anchorTime after every accepted in-band update. Repeated same-day checkpoints make later checks apply against a larger anchorRatio, so the per-day linear allowance compounds instead of staying bounded by the configured linear budget.
Align the drift model with implementation:
- Require a minimum elapsed time between between-action checkpoints, or
- Keep a separate long-horizon reference ratio that only a corporate action may reset and validate against that too.
🤖 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 `@src/concrete/oracle/DIAVaultOracle.sol` around lines 580 - 606, Update
checkpointRatio so repeated permissionless checkpoints within the same
between-action period cannot compound the configured drift allowance: enforce a
minimum elapsed interval between accepted same-action checkpoints, or maintain a
corporate-action-reset reference ratio and validate new ratios against it as
well. Preserve immediate acceptance when the completed action count changes or
the anchor is unset, and update the relevant anchor state consistently.
| function _validateRatioInBand(MainStorage storage $, Float ratio) internal view { | ||
| // anchorTime <= block.timestamp by construction (stored from | ||
| // block.timestamp). uint32 bps * uint64 elapsed < 2^96 — no overflow. | ||
| // Elapsed wall-clock time since the anchor is the whole point of a | ||
| // linearly accrued drift allowance; sub-band miner drift is immaterial | ||
| // against an allowance measured in bps/day. Not a value/authorisation | ||
| // dependence on block.timestamp. | ||
| // slither-disable-next-line timestamp | ||
| uint256 elapsed = block.timestamp - $.anchorTime; | ||
| Float drift = LibDecimalFloat.div( | ||
| LibDecimalFloat.fromFixedDecimalLosslessPacked(uint256($.maxRatioDriftPerDayBps) * elapsed, 0), | ||
| LibDecimalFloat.fromFixedDecimalLosslessPacked(10_000 * 1 days, 0) | ||
| ); | ||
| Float one = LibDecimalFloat.packLossless(1, 0); | ||
| Float anchor = $.anchorRatio; | ||
| Float upper = LibDecimalFloat.mul(anchor, LibDecimalFloat.add(one, drift)); | ||
| Float lower = LibDecimalFloat.mul(anchor, LibDecimalFloat.sub(one, drift)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a fuzz test for the drift-band boundary math.
_validateRatioInBand takes arbitrary elapsed, maxRatioDriftPerDayBps, and live ratio values, and it performs float multiplication and subtraction that can cross zero. The context shows scenario tests such as testVaultRatioBandDriftWithinBandServed, which pin specific points. A fuzz test would pin the boundary itself.
Cover at minimum:
maxRatioDriftPerDayBpsacross its fulluint32range, including0andtype(uint32).max.elapsedvalues large enough to drive accrued drift past1, wherelowergoes non-positive and only the upper bound binds.- Ratios exactly on
upperandlower, to pin whether the boundary is inclusive.
As per coding guidelines: "Every feature must have tests, with no exceptions; use fuzz tests wherever appropriate, especially for functions accepting arbitrary inputs."
🤖 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 `@src/concrete/oracle/DIAVaultOracle.sol` around lines 624 - 640, Add a fuzz
test alongside the existing ratio-band scenario tests, such as
testVaultRatioBandDriftWithinBandServed, covering arbitrary uint32
maxRatioDriftPerDayBps values including zero and the maximum, elapsed values
that make drift exceed one, and live ratios exactly on both calculated upper and
lower boundaries. Assert the expected inclusive boundary behavior and verify
that when the lower bound becomes non-positive only the upper bound constrains
validation.
Source: Coding guidelines
c61b53b to
2b21afa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/src/concrete/oracle/DIAVaultOracle.t.sol (1)
392-404: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse an asset-backed donation fixture for the ratio regression.
testDonationMovesRatioAndIsServeddocuments thatsetTotalAssetsonly models a bare transfer, but it still calls the setter without asserting the donation effect ontotalSupply. Use a real ERC-20 balance transfer into the mock vault or rename the regression to describe a mockedtotalAssetschange.🤖 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 392 - 404, Update testDonationMovesRatioAndIsServed to model the donation with an actual ERC-20 transfer into the mock vault, and assert that totalSupply remains unchanged after the transfer. Keep the existing ratio assertions and oracle behavior checks intact.
🤖 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 472-473: Update the slither-disable-next-line comment immediately
before the timestamp check in DIAVaultOracle to include an inline explanation
identifying why Slither’s timestamp finding is a false positive; if the check is
not a false positive, remove the suppression instead.
- Around line 460-475: Normalize future-timestamp handling across
latestRoundData: update the oracle’s timestamp-derived metadata so accepted
future pushes return block.timestamp as updatedAt and use a stored monotonic
round counter for roundId and answeredInRound. Ensure the counter advances
across pushes, including timestamp regressions, and preserve the existing
staleness behavior.
---
Nitpick comments:
In `@test/src/concrete/oracle/DIAVaultOracle.t.sol`:
- Around line 392-404: Update testDonationMovesRatioAndIsServed to model the
donation with an actual ERC-20 transfer into the mock vault, and assert that
totalSupply remains unchanged after the transfer. Keep the existing ratio
assertions and oracle behavior checks intact.
🪄 Autofix
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: 5c7e741c-4449-4db4-92c7-beffa9f1df88
📒 Files selected for processing (2)
src/concrete/oracle/DIAVaultOracle.soltest/src/concrete/oracle/DIAVaultOracle.t.sol
| // A push timestamped at or before `now` applies the `maxAge` window. A | ||
| // push timestamped in the FUTURE (a feed running slightly ahead, or a | ||
| // chain-time regression / reorg) is treated as fresh (age 0), never | ||
| // stale: the `<= block.timestamp` guard short-circuits the subtraction | ||
| // so it can never underflow into a bare `Panic(0x11)` that integrators | ||
| // 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 it matches the fail-closed staleness | ||
| // exact-equality boundary, and matches the fail-closed staleness | ||
| // convention (the edge counts as stale). | ||
| // slither-disable-next-line timestamp | ||
| if (block.timestamp - uint256(timestamp) >= $.maxAge) revert DIAPriceStale(uint256(timestamp)); | ||
| if (uint256(timestamp) <= block.timestamp && block.timestamp - uint256(timestamp) >= $.maxAge) { | ||
| revert DIAPriceStale(uint256(timestamp)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C 8 --glob '*.sol' \
'latestRoundData\s*\(|AggregatorV2V3Interface|updatedAt|answeredInRound|roundId' . || true
rg -nP -C 8 --glob '*.sol' \
'block\.timestamp\s*-\s*(updatedAt|timestamp)|(updatedAt|timestamp)\s*-\s*block\.timestamp' . || trueRepository: ST0x-Technology/st0x.oracle
Length of output: 33115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/concrete/oracle/DIAVaultOracle.sol test/src/e2e/DIAStackE2E.t.sol test/src/concrete/oracle/DIAVaultOracle.t.sol src/mocks/*DIA* src/interface/IDIAOracleV2.sol \
test/src/mocks/*DIA* 2>/dev/null; do
[ -f "$f" ] || continue
echo "===== $f ====="
wc -l "$f"
rg -n 'function\s+\w+|event\s+\w+|msg\.timestamp|block\.timestamp|timestamp|setValue|diaOracle|IDIAOracleV2|update|pause|warp' "$f" | sed -n '1,220p'
done
echo "===== files matching mock DIA ====="
fd -i 'dia|DIA' . -e sol | sed -n '1,200p'
echo "===== timestamp subtraction / age checks in sol ====="
rg -n 'block\.timestamp\s*-\s*(updatedAt|startedAt|roundId|timestamp)|updatedAt\s*-\s*block\.timestamp|maxAge|timestamp' --glob '*.sol' . | sed -n '1,240p'Repository: ST0x-Technology/st0x.oracle
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== candidate files ====="
fd -i 'dia|DIA|DIAOracle|Diavault|Mock' . -e sol | sed -n '1,240p'
echo "===== DIAVaultOracle relevant sections ====="
sed -n '1,230p' src/concrete/oracle/DIAVaultOracle.sol | cat -n
sed -n '350,475p' src/concrete/oracle/DIAVaultOracle.sol | cat -n
echo "===== tests relevant sections ====="
sed -n '80,140p' test/src/e2e/DIAStackE2E.t.sol | cat -n
sed -n '660,730p' test/src/concrete/oracle/DIAVaultOracle.t.sol | cat -n
echo "===== mock interfaces/implementations ====="
for f in $(fd -i 'dia|DIA|Diavault' src test -e_sol); do
echo "--- $f"
sed -n '1,220p' "$f"
done | sed -n '1,360p'
echo "===== timestamp subtraction / age checks across sol ====="
rg -n 'block\.timestamp\s*-\s*(updatedAt|startedAt|roundId|timestamp)|updatedAt\s*-\s*block\.timestamp|maxAge|timestamp|answeredInRound|roundId' --glob '*.sol' . | sed -n '1,300p'Repository: ST0x-Technology/st0x.oracle
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== MockDIAOracle and DIAOracle interface ====="
sed -n '1,80p' test/mocks/MockDIAOracle.sol | cat -n
sed -n '1,60p' src/interface/IDIAOracleV2.sol | cat -n
echo "===== Slither timestamp suppression context ====="
sed -n '450,476p' src/concrete/oracle/DIAVaultOracle.sol | cat -n
echo "===== repository timestamp-subtraction consumers in Solidity ====="
rg -nC 5 --glob '*.sol' 'block\.timestamp\s*-\s*(updatedAt|startedAt)|updatedAt\s*-\s*block\.timestamp|timestamp\s*-\s*block\.timestamp|answeredInRound|roundId' . | sed -n '1,220p'
echo "===== relevant tests around round/update/monotonic/future ====="
sed -n '650,740p' test/src/concrete/oracle/DIAVaultOracle.t.sol | cat -n
rg -n --glob '*.sol' 'Future|future|later|lower|monotonic|updatedAt|roundId|latestRoundData' test/src --max-count 200Repository: ST0x-Technology/st0x.oracle
Length of output: 28321
Normalize latestRoundData() when accepting future DIA timestamps.
The staleness check now treats future timestamps as valid, but latestRoundData() returns the raw timestamp as updatedAt and the truncated uint80 timestamp as both roundId and answeredInRound. A future push can return updatedAt > block.timestamp, and a later push with a lower timestamp can lower roundId despite the documented timestamp-derived monotonic behavior. Return the current block.timestamp for updatedAt and a monotonic round counter stored across pushes, or add explicit regression coverage that future pushes update these fields safely when consumers rely on them.
🤖 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 `@src/concrete/oracle/DIAVaultOracle.sol` around lines 460 - 475, Normalize
future-timestamp handling across latestRoundData: update the oracle’s
timestamp-derived metadata so accepted future pushes return block.timestamp as
updatedAt and use a stored monotonic round counter for roundId and
answeredInRound. Ensure the counter advances across pushes, including timestamp
regressions, and preserve the existing staleness behavior.
…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
2b21afa to
e8b4d02
Compare
1 participant
Audit-fix group for the DIA oracle. Closes #261, #262, #274; partially addresses #272.
Rebased onto
mainnow that #280 (the cross-epoch invariant fix) has merged. Single commit.Panic(0x11).description()deviation and soften the interface NatSpec so they no longer contradict.=0.8.25instead of floating^0.8.25(rain per-file-kind convention) #272 (part, LOW) — floatIAggregatorV2V3.solpragma 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 onlyname()andsymbol(). It inherits OpenZeppelin ERC-4626's defaulttotalAssets(), which is rawIERC20(asset()).balanceOf(address(this)). Donations do move the share ratio.OffchainAssetReceiptVaultAuthorizerV1.authorize()returns unconditionally forTRANSFER_SHARESwhile 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
Dbuys at mostLTV × yourShareOfSupply × Dof extra borrowing power at a cost ofD— 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.
testDonationMovesRatioAndIsServedpins 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-contractandreuse lintall green locally before push.🤖 Generated with Claude Code