chore(fuzz): overhaul fuzzing to drive real code with failable oracles#458
Open
peer2f00l wants to merge 6 commits into
Open
chore(fuzz): overhaul fuzzing to drive real code with failable oracles#458peer2f00l wants to merge 6 commits into
peer2f00l wants to merge 6 commits into
Conversation
Rework the fuzz suite around PRINCIPLES.md: every target now drives the real production functions (BorrowPosition, Deposit, PricePair/Valuation, UsageCurve, the Soroban codecs, DepositMsg, the vault OpState machine) behind oracles that can actually fail — round-trips, metamorphic identities, and explicit safety invariants — instead of the previous toy reimplementations and `let _ =` no-ops. Delete the dead, never- registered targets (collateral/*, borrow/*, decimal_arithmetic/*, snapshot/*) and the off-chain liquidator simulations; add stateful and backstop targets (fuzz_vault_state_machine, fuzz_borrow_overflow, fuzz_supply_overflow, fuzz_snapshot, fuzz_deposit_msg). Where a target narrows its input domain to dodge an intentional overflow, the excluded region is backstopped by a boundary target plus a `#[should_panic]` unit test; fuzz_decimal_arithmetic instead drops its cap entirely and runs the full u128 domain (integer Decimals can't overflow U512). Pin the four bugs fuzzing surfaced (ENG-341 Piecewise underflow, ENG-342 liquidation denominator underflow, ENG-343 Valuation::ratio div0, ENG-345 codec over-allocation) with committed regression seeds and inline, narrowly-scoped skips; the ENG-341/342 aborts are asserted by new should_panic backstop tests, and the liquidations skip is narrowed to only the `1 < cr < mcr` band so healthy/underwater positions stay fuzzed. Add a nightly CI workflow (corpus cached, crashes uploaded + filed, not auto-committed) and the PRINCIPLES / README / AGENTS / TRIAGE / seeds docs that make the suite normative and reproducible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughPull Request Summary — Fuzzing Test Suite OverhaulScope
Key changes
Notable behavioral changes
Critical review checklist (high-priority items to inspect)
Additional notes
WalkthroughAdds a nightly/manual GitHub Actions fuzz workflow, per-target corpus caching, many rewritten property-based fuzz targets and boundary backstops, unit tests as fuzz backstops, and comprehensive fuzzing documentation, triage, and seed management. ChangesFuzzing Infrastructure & Target Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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 @.github/workflows/fuzz.yml:
- Around line 177-247: The step "Open issue on crash" currently embeds sensitive
repro data when CRASH_B64 is set and posts it via github.rest.issues.create;
change this so that when CRASH_B64 exists you DO NOT include the base64 payload
or exact reproduce commands in the public issue body (constructed in the script
that builds lines and calls github.rest.issues.create). Instead, replace the
embedded repro block (the branch guarded by CRASH_B64) with a short,
non-sensitive note stating that a repro is attached to a private artifact or
security workflow and include instructions to fetch it only via secure channels
(or create a private security issue/advisory using the same inputs), and ensure
the github.rest.issues.create call only posts the sanitized lines array when
CRASH_B64 is present.
- Around line 44-55: Validate and allowlist the user-supplied
github.event.inputs.targets tokens before building the matrix: run `cargo
+nightly fuzz list` to produce an allowed list, split the incoming `requested`
string into tokens, and for each token accept it only if it matches an allowed
target; build the `targets` variable from those validated tokens (or fail/ignore
invalid ones) and then create `json` from that safe `targets` when writing to
GITHUB_OUTPUT and echoing the matrix. Ensure you reference the `requested`,
`targets`, `cargo +nightly fuzz list`, `json`, and `GITHUB_OUTPUT` variables so
the change is localized to this block.
In `@common/src/interest_rate_strategy.rs`:
- Around line 272-276: The test
piecewise_new_underflows_when_base_exceeds_cross_term pins the exact panic
message ("arithmetic operation overflow"); change it to only assert that a panic
occurs by removing the expected parameter from #[should_panic] (i.e., use
#[should_panic] above the test) so Piecewise::new(dec!("0.5"), dec!("0.9"),
dec!("0.0"), dec!("0.1")) is still verified to panic without relying on
backend-specific panic text.
In `@common/src/supply.rs`:
- Around line 507-508: The test deposit_total_overflow_aborts currently uses
#[should_panic(expected = "attempt to add with overflow")], which ties the test
to a toolchain-specific panic message; change the attribute to a non-specific
panic assertion (e.g., #[should_panic]) or otherwise remove the expected= string
so the test simply asserts that an overflow causes a panic without matching
exact text. Ensure this update is applied to the deposit_total_overflow_aborts
test function so it still fails on no-panic but is tolerant to different panic
messages.
In `@fuzz/fuzz_targets/fuzz_accumulator.rs`:
- Around line 34-40: Before calling accumulator.remove(removable) read let
prev_total = accumulator.get_total(); compute let expected_after_remove =
prev_total.saturating_sub(removable) (or prev_total - removable if totals are
guaranteed >= removable), then call accumulator.remove(removable) and replace
assert!(accumulator.get_total() <= add_a) with
assert_eq!(accumulator.get_total(), expected_after_remove); next, after
accumulator.add_once(add_a) compute let expected_after_add =
expected_after_remove + add_a and replace assert!(accumulator.get_total() >=
add_a) with assert_eq!(accumulator.get_total(), expected_after_add); reference
the existing symbols removable, add_a, accumulator.remove, accumulator.add_once
and accumulator.get_total when making these changes.
In `@fuzz/fuzz_targets/fuzz_borrow_invariants.rs`:
- Around line 23-30: The fuzz target's input tuple includes unused dimensions
`_b2` and `has_timestamp`; remove them from the fuzz_target! input signature and
from the destructuring so the tuple only contains the used fields
(snapshot_index, c1, c2, b1, in_flight), and update the subsequent conversion
that constructs u128 values for c1, c2, b1, and in_flight in the
fuzz_borrow_invariants fuzz target (the fuzz_target! invocation and its local
let (...) = data destructure). Apply the same removal/update to the other
occurrences of this fuzz target logic later in the file (the other instance
referenced around the second block).
In `@fuzz/fuzz_targets/fuzz_borrow_overflow.rs`:
- Around line 38-55: The fuzz target currently takes three u128s and sets
position.collateral_asset_deposit even though the oracles only depend on
borrow_asset_principal and borrow_asset_in_flight; change the fuzz input to a
2-tuple (u128, u128) and destructure to (principal, in_flight), remove the
collateral variable and the line that sets position.collateral_asset_deposit (or
set it to a constant like CollateralAssetAmount::new(0)), keeping the rest
(BorrowPosition::new(0), borrow_asset_principal and borrow_asset_in_flight
assignments and the checked_add guard) unchanged so the fuzzer concentrates its
dimension on the overflow boundary.
In `@fuzz/fuzz_targets/fuzz_borrow.rs`:
- Around line 22-29: Remove the unused fuzz dimensions by changing the fuzz
target input tuple from five values to only the three that matter (the
collateral/principal/in_flight u64s), update the destructuring to drop
snapshot_index and timestamp_ms, and adjust the subsequent conversion block that
converts those three u64s into u128s; also remove or parameterize the constant
check that repeatedly constructs BorrowPosition::new(u32::MAX) (the repeated
constant check in the harness should be eliminated or made dependent on the
fuzzed inputs) so the corpus bytes are dedicated to the meaningful
principal/in_flight space (apply the same removals/adjustments to the similar
block around the BorrowPosition usage at lines ~62-73).
In `@fuzz/fuzz_targets/fuzz_decimal_parsing.rs`:
- Around line 44-50: The overflow guard currently uses `if decimal <=
Decimal::MAX / Decimal::TWO` for both `+ Decimal::ONE` and `* Decimal::TWO`,
which prevents exercising parsed values in (MAX/2, MAX-1] for the `+ ONE` case;
split the guards so addition and multiplication have separate checks: use `if
decimal < Decimal::MAX` (or `<= Decimal::MAX - Decimal::ONE`) before `let _ =
decimal + Decimal::ONE;` and keep `if decimal <= Decimal::MAX / Decimal::TWO`
before `let _ = decimal * Decimal::TWO;` so `decimal` in the upper half of the
domain still exercises the addition path.
In `@fuzz/fuzz_targets/fuzz_decimals.rs`:
- Around line 57-61: The check for the zero-usage endpoint is gated on data.4 (a
1-in-2^64 condition) so it almost never runs; replace the conditional branch and
instead unconditionally call piecewise.at(Decimal::ZERO) and assert it equals
base_p (use the same message SUT "Piecewise::at(0) must equal base") every
iteration; do the same fix for the duplicate check around the second occurrence
(the lines analogous to 88-90) so both places use piecewise.at(Decimal::ZERO)
and assert_eq!(..., base_p).
In `@fuzz/fuzz_targets/fuzz_market_creation.rs`:
- Around line 200-213: Add a deterministic positive-control assertion to ensure
we catch regressions that over-reject: construct one known-valid
MarketConfiguration (e.g., the same parameters used in existing successful tests
or a minimal valid config) and assert that calling
MarketConfiguration::validate() on it returns Ok (validation_result.is_ok() or
explicit call), while keeping the existing downgrade that only requires no panic
for arbitrary inputs; locate this near the existing validation_result usage in
fuzz_market_creation.rs so the harness still performs the negative checks but
also fails if validate() always returns Err.
In `@fuzz/fuzz_targets/fuzz_snapshot.rs`:
- Around line 84-109: The test deserializes JSON into a Snapshot (variable d)
but never validates the Decimal field interest_rate, so JSON bugs can slip
through; after serde_json::from_str(&json) and existing asserts, re-serialize d
with serde_json::to_string and compare that JSON to the original json (or at
minimum deserialize both JSON strings and assert equality of the interest_rate
field), ensuring the interest_rate serialized form (or value) round-trips
through serde_json; update fuzz_targets/fuzz_snapshot.rs to reference Snapshot,
d, json, and interest_rate when adding this extra assertion.
In `@fuzz/fuzz_targets/fuzz_vault_state_machine.rs`:
- Around line 114-116: The OpState::Withdrawing branch only asserts op_id but
doesn't verify that the WithdrawingState.amount_collected never exceeds
WithdrawingState::remaining; update the check in the OpState::Withdrawing arm
(where you currently call state.op_id()) to also locate the WithdrawingState (s)
and assert that s.amount_collected <= s.remaining (or that the state's recorded
amount_collected value is <= WithdrawingState::remaining) so any excess
accumulation by withdrawal_step_callback is detected; ensure you reference
WithdrawingState::remaining and the amount_collected field on the same state
instance when adding the assertion.
- Around line 150-245: The test only verifies that Errored transitions don't
mutate state but doesn't assert that invalid actions (callbacks,
complete_allocation from Idle/non-Allocating, or mismatched op_id) must fail;
update the loop over scenario.actions so that for Action variants that should be
rejected (AllocationStepCallback, RefreshStepCallback, WithdrawalStepCallback,
WithdrawalCollected, WithdrawalSettled, CompleteAllocation when with_withdrawal
is Some with mismatched request.op_id, and any callback/payout variants when
state.kind_code() is not the corresponding active kind or state.op_id() !=
op_id) you assert result.is_err() before allowing Ok transitions; use the
existing symbols (Action::AllocationStepCallback, Action::CompleteAllocation,
allocation_step_callback, complete_allocation, withdrawal_step_callback,
withdrawal_collected, withdrawal_settled, refresh_step_callback,
payout_complete, state.kind_code(), state.op_id()) to detect invalid
preconditions and fail the test if they returned Ok.
In `@fuzz/README.md`:
- Around line 62-83: Update the "Bugs found by fuzzing" / status table in
README.md to record the removed liquidator fuzz coverage: add a new row
documenting the dropped liquidator target (mention it was removed from
fuzz/Cargo.toml), include a brief bug description like "liquidator fuzzers
removed — coverage reduced" and link to the tracking issue (create or reference
the Linear issue key provided by the reviewer), and indicate which fuzzer
previously found it (e.g., `fuzz_liquidations`) so the coverage reduction is
visible in the table.
In `@fuzz/TRIAGE.md`:
- Around line 81-148: Add missing blank lines around the level-3 headings (e.g.,
"### Outcome 1 — Real bug", "### Outcome 2 — Intentional abort", "### Outcome 3
— Harness bug", and the "### 5. Make it durable" section) so each `###` heading
is preceded and followed by a blank line to satisfy MD022, and change the final
quick-reference fenced block (the bash/code snippet under "5a. Promote the
input...") to use a typed fence like ```text so it no longer triggers MD040;
apply the same blank-line and fence-type fixes to the other similar block noted
in the comment (the 163-172 region).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d74705f-c2ed-4719-9411-20536af87886
📒 Files selected for processing (74)
.github/workflows/fuzz.ymlcommon/src/borrow.rscommon/src/interest_rate_strategy.rscommon/src/supply.rsfuzz/AGENTS.mdfuzz/Cargo.tomlfuzz/PRINCIPLES.mdfuzz/README.mdfuzz/TRIAGE.mdfuzz/fuzz_targets/borrow/borrow_all_amounts_set.rsfuzz/fuzz_targets/borrow/borrow_collateral.rsfuzz/fuzz_targets/borrow/borrow_existence.rsfuzz/fuzz_targets/borrow/borrow_inflight_amounts.rsfuzz/fuzz_targets/borrow/borrow_liability.rsfuzz/fuzz_targets/borrow/borrow_liquidation_lock.rsfuzz/fuzz_targets/borrow/borrow_max_snapshot.rsfuzz/fuzz_targets/borrow/borrow_overflow.rsfuzz/fuzz_targets/borrow/borrow_status_invariants.rsfuzz/fuzz_targets/borrow/borrow_zero_amounts.rsfuzz/fuzz_targets/decimal_arithmetic/basic_arithmetic.rsfuzz/fuzz_targets/decimal_arithmetic/comparisons.rsfuzz/fuzz_targets/decimal_arithmetic/constants.rsfuzz/fuzz_targets/decimal_arithmetic/conversions.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/alphabetic_chars.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/invalid_separators.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/mixed_invalid.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/multiple_decimals.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/scientific_notation.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/sign_errors.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/special_values.rsfuzz/fuzz_targets/decimal_arithmetic/malformed/whitespace.rsfuzz/fuzz_targets/decimal_arithmetic/parsing_conversions.rsfuzz/fuzz_targets/decimal_arithmetic/parsing_edge_cases.rsfuzz/fuzz_targets/decimal_arithmetic/parsing_large_numbers.rsfuzz/fuzz_targets/decimal_arithmetic/parsing_patterns.rsfuzz/fuzz_targets/decimal_arithmetic/parsing_precision.rsfuzz/fuzz_targets/decimal_arithmetic/parsing_roundtrip.rsfuzz/fuzz_targets/decimal_arithmetic/parsing_utf8.rsfuzz/fuzz_targets/decimal_arithmetic/powers.rsfuzz/fuzz_targets/decimal_arithmetic/scaling.rsfuzz/fuzz_targets/decimal_arithmetic/strings.rsfuzz/fuzz_targets/decimal_arithmetic/utilities.rsfuzz/fuzz_targets/fuzz_accumulator.rsfuzz/fuzz_targets/fuzz_borrow.rsfuzz/fuzz_targets/fuzz_borrow_invariants.rsfuzz/fuzz_targets/fuzz_borrow_overflow.rsfuzz/fuzz_targets/fuzz_collateral_combined.rsfuzz/fuzz_targets/fuzz_collateral_deposit.rsfuzz/fuzz_targets/fuzz_collateral_ops.rsfuzz/fuzz_targets/fuzz_collateral_overflow.rsfuzz/fuzz_targets/fuzz_decimal_arithmetic.rsfuzz/fuzz_targets/fuzz_decimal_parsing.rsfuzz/fuzz_targets/fuzz_decimals.rsfuzz/fuzz_targets/fuzz_deposit_msg.rsfuzz/fuzz_targets/fuzz_interest_math.rsfuzz/fuzz_targets/fuzz_liquidations.rsfuzz/fuzz_targets/fuzz_liquidator_logic.rsfuzz/fuzz_targets/fuzz_liquidator_transactions.rsfuzz/fuzz_targets/fuzz_market_creation.rsfuzz/fuzz_targets/fuzz_price.rsfuzz/fuzz_targets/fuzz_price_calculations.rsfuzz/fuzz_targets/fuzz_snapshot.rsfuzz/fuzz_targets/fuzz_soroban_storage_codecs.rsfuzz/fuzz_targets/fuzz_supply.rsfuzz/fuzz_targets/fuzz_supply_overflow.rsfuzz/fuzz_targets/fuzz_vault_state_machine.rsfuzz/fuzz_targets/snapshot/fuzz_snapshot_creation.rsfuzz/run_fuzzing.shfuzz/seeds/README.mdfuzz/seeds/fuzz_decimals/ENG-341-piecewise-underflow-1fuzz/seeds/fuzz_decimals/ENG-341-piecewise-underflow-2fuzz/seeds/fuzz_liquidations/ENG-342-denominator-underflowfuzz/seeds/fuzz_price/ENG-343-ratio-div0-1fuzz/seeds/fuzz_price/ENG-343-ratio-div0-2
💤 Files with no reviewable changes (40)
- fuzz/fuzz_targets/borrow/borrow_zero_amounts.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/whitespace.rs
- fuzz/fuzz_targets/decimal_arithmetic/parsing_roundtrip.rs
- fuzz/fuzz_targets/borrow/borrow_liability.rs
- fuzz/fuzz_targets/fuzz_collateral_overflow.rs
- fuzz/fuzz_targets/decimal_arithmetic/conversions.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/mixed_invalid.rs
- fuzz/fuzz_targets/borrow/borrow_all_amounts_set.rs
- fuzz/fuzz_targets/fuzz_collateral_combined.rs
- fuzz/fuzz_targets/decimal_arithmetic/constants.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/special_values.rs
- fuzz/fuzz_targets/fuzz_collateral_deposit.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/alphabetic_chars.rs
- fuzz/fuzz_targets/fuzz_collateral_ops.rs
- fuzz/fuzz_targets/borrow/borrow_liquidation_lock.rs
- fuzz/fuzz_targets/borrow/borrow_inflight_amounts.rs
- fuzz/fuzz_targets/decimal_arithmetic/strings.rs
- fuzz/fuzz_targets/borrow/borrow_existence.rs
- fuzz/fuzz_targets/snapshot/fuzz_snapshot_creation.rs
- fuzz/fuzz_targets/decimal_arithmetic/powers.rs
- fuzz/fuzz_targets/decimal_arithmetic/parsing_patterns.rs
- fuzz/fuzz_targets/decimal_arithmetic/parsing_precision.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/invalid_separators.rs
- fuzz/fuzz_targets/borrow/borrow_max_snapshot.rs
- fuzz/fuzz_targets/decimal_arithmetic/scaling.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/multiple_decimals.rs
- fuzz/fuzz_targets/borrow/borrow_collateral.rs
- fuzz/fuzz_targets/decimal_arithmetic/parsing_large_numbers.rs
- fuzz/fuzz_targets/borrow/borrow_status_invariants.rs
- fuzz/fuzz_targets/fuzz_liquidator_transactions.rs
- fuzz/fuzz_targets/decimal_arithmetic/parsing_utf8.rs
- fuzz/fuzz_targets/decimal_arithmetic/parsing_edge_cases.rs
- fuzz/fuzz_targets/decimal_arithmetic/comparisons.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/sign_errors.rs
- fuzz/fuzz_targets/borrow/borrow_overflow.rs
- fuzz/fuzz_targets/decimal_arithmetic/malformed/scientific_notation.rs
- fuzz/fuzz_targets/fuzz_liquidator_logic.rs
- fuzz/fuzz_targets/decimal_arithmetic/basic_arithmetic.rs
- fuzz/fuzz_targets/decimal_arithmetic/utilities.rs
- fuzz/fuzz_targets/decimal_arithmetic/parsing_conversions.rs
Address unresolved CodeRabbit review threads on PR #458: - fuzz_accumulator: assert exact totals after remove/re-add (was weak <=/>=) - fuzz_decimal_parsing: split overflow guards so `+ ONE` covers the upper half - fuzz_decimals: assert at(0)==base every iteration (was a 1-in-2^64 gate) - fuzz_market_creation: add a known-valid positive-control config oracle - fuzz_borrow_invariants: drop dead `_b2` / `has_timestamp` dimensions - fuzz_borrow_overflow: drop the unused `collateral` dimension - fuzz_borrow: drop dead `snapshot_index` / `timestamp_ms` + constant block - fuzz.yml: allowlist workflow_dispatch targets via env (template injection) - README/TRIAGE: record removed liquidator coverage (ENG-344); markdownlint Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/fuzz.yml (1)
33-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin GitHub Actions to full commit SHAs in
.github/workflows/fuzz.yml
The workflow still uses mutable action refs (not commit SHAs), which could be retagged upstream and execute with CI privileges:
actions/checkout@v4(33, 83)dtolnay/rust-toolchain@nightly(35, 86)taiki-e/install-action@v2(38, 90)Swatinem/rust-cache@v2(99)actions/cache/restore@v4(110)actions/cache/save@v4(149)actions/upload-artifact@v4(156)actions/github-script@v7(192)
Replace each with a reviewed full-length commit SHA (e.g.,@<40-hex>).🤖 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 @.github/workflows/fuzz.yml around lines 33 - 38, The workflow uses mutable action refs (examples: actions/checkout@v4, dtolnay/rust-toolchain@nightly, taiki-e/install-action@v2, Swatinem/rust-cache@v2, actions/cache/restore@v4, actions/cache/save@v4, actions/upload-artifact@v4, actions/github-script@v7) which must be pinned to reviewed full 40-hex commit SHAs; update each uses: entry to replace the tag/branch ref with the corresponding repository's exact commit SHA (e.g., @<40-hex>) you verified from the action's upstream repo, commit the updated refs in .github/workflows/fuzz.yml, and ensure CI still passes.
♻️ Duplicate comments (1)
.github/workflows/fuzz.yml (1)
227-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't publish crash reproducers in the issue body.
This branch turns every scheduled fuzz finding with a small input into a public, copy-pasteable reproducer. Keep the issue sanitized and move the payload to a private/security-only channel, or at minimum stop appending
CRASH_B64and the exact replay commands tolines.🔒 Minimal sanitizing change
if (CRASH_B64) { - // Self-contained repro: the input is embedded, no artifact needed. lines.push( - `Reproduce (input embedded below):`, - ``, - '```bash', - `echo '${CRASH_B64}' | base64 -d > /tmp/${TARGET}-crash`, - `cargo +nightly fuzz run ${TARGET} /tmp/${TARGET}-crash`, - '```', + `A minimized reproducer was captured for this crash, but it is intentionally not embedded in this public issue.`, + `Retrieve the input through the repository's private triage channel before investigating.`, ); } else {Based on learnings: Treat every code change as potentially security-relevant in this security-sensitive codebase.
🤖 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 @.github/workflows/fuzz.yml around lines 227 - 236, The workflow currently appends the full CRASH_B64 and exact replay commands into the public issue by pushing lines containing `echo '${CRASH_B64}' | base64 -d > /tmp/${TARGET}-crash` and `cargo +nightly fuzz run ${TARGET} /tmp/${TARGET}-crash`; change the `lines.push` call so it does NOT include `CRASH_B64` or the exact replay commands and instead inserts a sanitized message (e.g., "A minimized reproducer was captured but is intentionally not embedded in this public issue. Retrieve the input through the repository's private triage channel.") along with guidance to use the repo's private/security channel to obtain the repro; keep references to `TARGET` only in generic wording, and ensure no direct base64 or replay command strings referencing `CRASH_B64` are appended to `lines`.
🤖 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.
Outside diff comments:
In @.github/workflows/fuzz.yml:
- Around line 33-38: The workflow uses mutable action refs (examples:
actions/checkout@v4, dtolnay/rust-toolchain@nightly, taiki-e/install-action@v2,
Swatinem/rust-cache@v2, actions/cache/restore@v4, actions/cache/save@v4,
actions/upload-artifact@v4, actions/github-script@v7) which must be pinned to
reviewed full 40-hex commit SHAs; update each uses: entry to replace the
tag/branch ref with the corresponding repository's exact commit SHA (e.g.,
@<40-hex>) you verified from the action's upstream repo, commit the updated refs
in .github/workflows/fuzz.yml, and ensure CI still passes.
---
Duplicate comments:
In @.github/workflows/fuzz.yml:
- Around line 227-236: The workflow currently appends the full CRASH_B64 and
exact replay commands into the public issue by pushing lines containing `echo
'${CRASH_B64}' | base64 -d > /tmp/${TARGET}-crash` and `cargo +nightly fuzz run
${TARGET} /tmp/${TARGET}-crash`; change the `lines.push` call so it does NOT
include `CRASH_B64` or the exact replay commands and instead inserts a sanitized
message (e.g., "A minimized reproducer was captured but is intentionally not
embedded in this public issue. Retrieve the input through the repository's
private triage channel.") along with guidance to use the repo's private/security
channel to obtain the repro; keep references to `TARGET` only in generic
wording, and ensure no direct base64 or replay command strings referencing
`CRASH_B64` are appended to `lines`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce1ed12c-8cb9-4177-b1dc-4384f6fefcc6
📒 Files selected for processing (10)
.github/workflows/fuzz.ymlfuzz/README.mdfuzz/TRIAGE.mdfuzz/fuzz_targets/fuzz_accumulator.rsfuzz/fuzz_targets/fuzz_borrow.rsfuzz/fuzz_targets/fuzz_borrow_invariants.rsfuzz/fuzz_targets/fuzz_borrow_overflow.rsfuzz/fuzz_targets/fuzz_decimal_parsing.rsfuzz/fuzz_targets/fuzz_decimals.rsfuzz/fuzz_targets/fuzz_market_creation.rs
- fuzz_vault_state_machine: assert wrong-state ⇒ Err (WrongState) and mismatched-op_id ⇒ Err (OpIdMismatch); transitions check state kind then op_id before any Ok path, so these hold without false positives (fuzz-verified ~3.6M cases) - interest_rate_strategy / supply: relax #[should_panic] backstops to the stable "overflow" substring instead of the full toolchain-specific text - fuzz.yml: don't embed the crash reproducer in the public crash issue; point to the run artifact instead (see ENG-349 for the follow-up) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rework the fuzz suite around PRINCIPLES.md: every target now drives the real production functions (BorrowPosition, Deposit, PricePair/Valuation, UsageCurve, the Soroban codecs, DepositMsg, the vault OpState machine) behind oracles that can actually fail — round-trips, metamorphic identities, and explicit safety invariants — instead of the previous toy reimplementations and
let _ =no-ops. Delete the dead, never- registered targets (collateral/, borrow/, decimal_arithmetic/, snapshot/) and the off-chain liquidator simulations; add stateful and backstop targets (fuzz_vault_state_machine, fuzz_borrow_overflow, fuzz_supply_overflow, fuzz_snapshot, fuzz_deposit_msg). Where a target narrows its input domain to dodge an intentional overflow, the excluded region is backstopped by a boundary target plus a#[should_panic]unit test; fuzz_decimal_arithmetic instead drops its cap entirely and runs the full u128 domain (integer Decimals can't overflow U512).Pin the four bugs fuzzing surfaced (ENG-341 Piecewise underflow, ENG-342 liquidation denominator underflow, ENG-343 Valuation::ratio div0, ENG-345 codec over-allocation) with committed regression seeds and inline, narrowly-scoped skips; the ENG-341/342 aborts are asserted by new should_panic backstop tests, and the liquidations skip is narrowed to only the
1 < cr < mcrband so healthy/underwater positions stay fuzzed. Add a nightly CI workflow (corpus cached, crashes uploaded + filed, not auto-committed) and the PRINCIPLES / README / AGENTS / TRIAGE / seeds docs that make the suite normative and reproducible.This change is