fix: enforce the DecimalsTooLarge bound inside LibFtsoCurrentPriceUsd - #137
fix: enforce the DecimalsTooLarge bound inside LibFtsoCurrentPriceUsd#137thedavidmeister wants to merge 11 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThe FTSO price library now rejects decimals above ChangesOracle Validation Updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Update BYTECODE_HASH after ZeroSFLRRate guard changes compiled bytecode. BuildPointers.sol produces 0x20a972f9f2c50d92ba72839450586e0bcdfdcaf8400f2f987c3bf9783075b7fc. Co-Authored-By: Claude <noreply@anthropic.com>
Use vm.envString("FLARE_RPC_URL") — CI maps the secret to FLARE_RPC_URL,
not RPC_URL_FLARE_FORK; the old envOr fell back to Ankr causing rate
limit failures on fork tests.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
Rework note (human reject, 2026-07-04): all three fixes are substantively right (bytes("") OutOfFunds pin per the audit rule; DecimalsTooLarge relocated into the library; ZeroSFLRRate fail-loud guard) but the PR cannot land as-is: (1) it ADDS an |
Clean merge of origin/main; pointer/meta regen against the merged source produced no drift (committed artifacts already consistent). The LibFork hunk from this branch is byte-identical to the change landed via #151 and dissolves into main. Co-Authored-By: Claude <noreply@anthropic.com>
The @Custom:error NatSpec tag is banned org-wide by the no-custom-natspec static gate; document the revert condition as prose instead. Co-Authored-By: Claude <noreply@anthropic.com>
The earlier merge in this branch used a stale single-branch fetch of main; this merges the current main tip. The generated pointers and meta artifacts are resolved by regenerating against the merged source via script/build.sh (BuildAuthoringMeta + rain meta build + BuildPointers, run to convergence). Co-Authored-By: Claude <noreply@anthropic.com>
Union-preserving resolution of ErrFtso.sol comment wording, equivalent test spellings in LibFtsoV2LTS.t.sol and LibSceptreStakedFlare.t.sol (both sides implement identical coverage; main's stricter mock kept), and regenerated src/generated/FlareFtsoWords.pointers.sol via rain-flare-prelude to its fixed point on the merged tree. Co-Authored-By: Claude <noreply@anthropic.com>
|
🤖 ai:vetter |
… bound Adds test/src/lib/price/LibFtsoCurrentPriceUsd.t.sol, which calls the library directly rather than through LibOpFtsoCurrentPriceUsd, so the bound is asserted at the trust boundary that now enforces it: - happy path returns (price, decimals) unchanged for decimals <= uint8 max - decimals > uint8 max reverts DecimalsTooLarge(decimals) - decimals == type(uint8).max is accepted - decimals == type(uint8).max + 1 reverts - a stale price with oversized decimals still reports StalePrice Updates the NatSpec on both sides of the move: the library now documents that it reverts DecimalsTooLarge and that the returned decimals are bounded, and the op documents DecimalsTooLarge as propagated rather than raised locally. Co-Authored-By: Claude <noreply@anthropic.com>
|
CI state at head
The test job's 19 failures are all the same non-code fault: the fork provider behind Every non-fork test passed, including all 8 in the new suite: Locally, against a working Flare RPC, the full suite including the fork and prod tests is 93 passed / 0 failed ( No rerun was triggered and no retrigger commit was pushed; the red needs the provider to recover, not a code change. This PR changes bytecode ( |
|
🤖 ai:vetter |
|
🤖 ai:producer |
Summary
Moves the
DecimalsTooLargeguard fromLibOpFtsoCurrentPriceUsd.run(the caller) intoLibFtsoCurrentPriceUsd.ftsoCurrentPriceUsd(the library that actually reads the FTSO), and covers it with a test that calls the library directly.The FTSO is untrusted. Bounding its reported
decimalsat one call site by authorship accident means any future direct caller of the library silently downcasts an oversizeddecimalsfrom a malicious or buggy FTSO and mis-scales the price by orders of magnitude. The bound belongs at the trust boundary, so the caller's redundant check is replaced by a comment recording that the library guarantees it.NatSpec is corrected on both sides of the move: the library now documents that it reverts
DecimalsTooLargeand that the returneddecimalsis always<= type(uint8).max; the op documentsDecimalsTooLargeas propagated rather than raised locally.src/generated/FlareFtsoWords.pointers.solis regenerated — this changes bytecode.Scope changed after merge-update
This branch was 16 commits behind
main. Two of the three fixes it originally carried landed independently while it sat:vm.expectRevert()in the paid-feed test) — already onmain;test/src/lib/lts/LibFtsoV2LTS.t.solusesvm.expectRevert(new bytes(0)). Issue is CLOSED.Closes #78dropped from this body: this PR no longer contains that change.main;LibSceptreStakedFlare.getSFLRPerFLR18already hasif (rate == 0) revert ZeroSFLRRate();. Issue is CLOSED.Closes #80dropped for the same reason. The one line of this diff still touchingLibSceptreStakedFlare.solis only the NatSpec sentence documenting that revert, which did not land with the guard.Closes #79
QA
Discriminating tests (all in the new
test/src/lib/price/LibFtsoCurrentPriceUsd.t.sol, all calling the library directly via an external wrapper sovm.expectRevertmonitors the right call frame, all withabi.encodeWithSelector(...)expectations — never barevm.expectRevert()):testFtsoCurrentPriceUsdDecimalsTooLargeReverts— fuzzeddecimals ∈ [256, int32.max]must revertDecimalsTooLarge(decimals). Fails on base: onmainthe library has no guard at all, so the call returns normally. Verified by mutation M1, which reproduces exactly the pre-PR library.testFtsoCurrentPriceUsdDecimalsBoundaryOverMaxReverts—decimals == type(uint8).max + 1exactly must revertDecimalsTooLarge(256). Fails on base (M1).testFtsoCurrentPriceUsdDecimalsBoundaryMaxAccepted—decimals == type(uint8).maxexactly must be accepted and returned verbatim. Pins the comparison against being tightened to>=(M2).testFtsoCurrentPriceUsdStaleTakesPrecedenceOverDecimals— a stale price that also reports oversized decimals must reportStalePrice, notDecimalsTooLarge. Pins the guard's position after the staleness check so it cannot mask an earlier failure (M3).testFtsoCurrentPriceUsdHappy— fuzzed in-bound case:(price, decimals)returned unchanged. The library bounds, it never rescales (M2, M5).FtsoTest, so the inherited registry-failure tests (testRunNoRegistry,testRunRegistryNoFtsoRegistry,testRunInvalidFtso) now also run against the library directly.Mutations applied to
src/lib/price/LibFtsoCurrentPriceUsd.sol, each compiled and run againstLibFtsoCurrentPriceUsdTest+LibOpFtsoCurrentPriceUsdTest+LibOpFtsoCurrentPricePairTest, then reverted. Baseline verified green immediately before the first mutation and again after the last restore (rc=0, 0 failures both times). No mutant survived:main)testFtsoCurrentPriceUsdDecimalsTooLargeReverts,testFtsoCurrentPriceUsdDecimalsBoundaryOverMaxReverts,testRunDecimalOverflow,testRunPairDecimalsTooLargeFirstLeg,testRunPairDecimalsTooLargeSecondLegdecimals > type(uint8).max→>=testFtsoCurrentPriceUsdDecimalsBoundaryMaxAccepted,testFtsoCurrentPriceUsdHappy,testRunDecimalsBoundary,testRunHappy,testRunHappyTrustedAddresses,testRunFtsoNotActiveAtestFtsoCurrentPriceUsdStaleTakesPrecedenceOverDecimals,testRunStalerevert DecimalsTooLarge(decimals)→DecimalsTooLarge(0)testFtsoCurrentPriceUsdDecimalsTooLargeReverts,testFtsoCurrentPriceUsdDecimalsBoundaryOverMaxReverts,testRunDecimalOverflow,testRunPairDecimalsTooLargeFirstLeg,testRunPairDecimalsTooLargeSecondLegreturn (price, decimals)→return (price, 0)testFtsoCurrentPriceUsdDecimalsBoundaryMaxAccepted,testFtsoCurrentPriceUsdHappy,testRunDecimalsBoundary,testRunHappy,testRunHappyTrustedAddresses,testRunStaleBoundaryNotStale,testRunPairDerivationExact,testRunPairZeroQuoteRevertsOracle:
type(uint8).maxis the largest value that survives theuint8(decimals)downcast at the only consumer of this return value (LibDecimalFloat.fromFixedDecimalLosslessPacked(price, uint8(decimals))), so 255 must pass and 256 must revert — derived from the downcast width, not from reading the guard. The FTSO-side values (price,timestamp,decimals) are supplied byvm.mockCall, so the expected outputs are fixed by the test, not by the implementation. The two boundary tests are hardcoded, not fuzzed, so they land on the boundary every run.Category check: [F28] [LOW] Library returns un-bounded decimals; DecimalsTooLarge guard is the caller's responsibility and undocumented #79 asks for the
DecimalsTooLargebound to be enforced at the library trust boundary rather than left to callers (its option (a)), and separately notes the risk is undocumented. Both are covered: the guard moved, and the NatSpec on the library now states the revert and the<= type(uint8).maxpostcondition. Option (b) (document-only) was not taken; (a) is what the issue prefers for an oracle library. The caller path is unchanged in behavior —testRunDecimalOverflow,testRunDecimalsBoundaryand the pair op'stestRunPairDecimalsTooLarge*still pass, now via propagation.Pointers:
src/generated/FlareFtsoWords.pointers.solwas regenerated, not text-merged. Rannix develop -c rain-flare-prelude(./script/build.sh, the same reciperainix-copy-artifactsruns) after mergingmain; it produced zero diff against the committed file. To prove that is a real check and not a silent no-op,BYTECODE_HASHwas deliberately corrupted andforge script ./script/Build.solre-run — it restored the committed value byte-for-byte.Toolchain: everything run through the flake (
nix develop … -c), never bare PATH;rc=$?captured directly from each command.forge fmt --checkrc=0; fullforge test(including fork and prod suites) 93 passed / 0 failed,rc=0.Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests