fix(security): august 2026 audit fixes across runtime, codec, node and system contracts - #491
fix(security): august 2026 audit fixes across runtime, codec, node and system contracts#491dmitry123 wants to merge 32 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes add authenticated release verification and provenance tracking, harden codec decoding, add runtime memory and fuel controls, update upgrade-plan ownership behavior, adopt Osaka EIP-7951 accounting, validate contract metadata and ownership inputs, and expand integration coverage. ChangesRelease verification and genesis integration
Runtime and decoding safety
Contract behavior
Repository support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Criterion results (vs baseline)Heads-up: runner perf is noisy; treat deltas as a smoke check. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
crates/codec/src/hash.rs (1)
588-622: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe two tests are correct, but they do not cover the Solidity collection paths.
Both fixtures resolve to an empty body, so
validate_collection_bodyfails beforetry_reserve. The assertions are accurate for 64-bit and 32-bit targets.The Solidity
HashMapandHashSetdecoders received the same hardening at lines 264-292 and 514-541, and they usechecked_decode_slice_frominstead ofchecked_decode_slice. That path has no regression test here. Add one Solidity fixture for each type.🤖 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 `@crates/codec/src/hash.rs` around lines 588 - 622, Add regression tests covering the Solidity collection decoders for both HashMap and HashSet, alongside the existing compact tests. Build fixtures with an oversized claimed count and headers that exercise the checked_decode_slice_from path in the Solidity implementations, then assert decoding fails with BufferTooSmall or Overflow before allocation. Use the existing Solidity decoder test patterns and retain separate tests for each collection type.crates/codec/src/vec.rs (1)
266-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth tests assert the fix, and the fixtures are consistent with the decoders.
The compact fixture makes
read_bytesreturn an empty body, sovalidate_collection_body(0xffff_ffff, 4, 0)fails beforetry_reserve. The Solidity fixture places a body offset of 32 and a count ofu32::MAX, sochecked_decode_slice_fromreturns an empty slice and validation fails. TheBufferTooSmall | Overflowpattern also covers 32-bit targets, wherechecked_muloverflows instead.Consider one addition: assert that a count that is one element larger than the body is also rejected. The current fixtures use an empty body, so they do not cover an off-by-one boundary.
🤖 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 `@crates/codec/src/vec.rs` around lines 266 - 296, Extend both compact and Solidity vector decoder tests to cover the off-by-one boundary where the declared element count is exactly one greater than the available body capacity. Keep the existing empty-body and error assertions, and add fixtures/assertions that confirm this count is rejected before allocation.crates/codec/src/encoder.rs (1)
262-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRequire contiguous input buffers for decoded slices.
checked_decode_sliceandchecked_decode_slice_fromcomparechunk.len()instead ofbuf.remaining(), while nearby decode helpers usebuf.remaining()only to validate bounds before readingchunk[...]. Document these as contiguous-buffer-only helpers, or unify the checks.🤖 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 `@crates/codec/src/encoder.rs` around lines 262 - 299, Clarify in the documentation for checked_decode_slice and checked_decode_slice_from that they require the requested range to be available in the current contiguous buf.chunk(), or update both helpers to validate bounds against buf.remaining() consistently with nearby decode helpers before slicing chunk. Preserve the existing overflow and BufferTooSmall error behavior.crates/release-verify/src/load.rs (1)
205-224: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse a unique temporary file name in
write_atomic.The temp path is always
<path>.tmp. If two processes share the cache directory, both write the same temp file and both rename it, so the cache can receive interleaved content. A later run re-authenticates the cache and rejects it, so the effect is a wasted re-download rather than acceptance of bad bytes. Add the process id, or usetempfile::NamedTempFile::persistin the same directory.♻️ Proposed fix for concurrent cache writes
pub fn write_atomic(path: &Path, bytes: &[u8]) -> Result<()> { let mut tmp_name = OsString::from(path.as_os_str()); - tmp_name.push(".tmp"); + tmp_name.push(format!(".{}.tmp", std::process::id())); let tmp = PathBuf::from(tmp_name);A failed write then leaves a process-specific temp file; remove it on the error paths if that matters.
🤖 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 `@crates/release-verify/src/load.rs` around lines 205 - 224, Update write_atomic to create a process-specific temporary path in the same directory, incorporating the current process id into the existing “.tmp” name or using tempfile::NamedTempFile with persist. Ensure all create, write, sync, and rename operations use this unique path, preserving the existing atomic replacement and error reporting behavior.crates/release-verify/src/tests.rs (1)
580-610: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the published manifest signature cryptographically.
This test checks signature type, digest algorithm, and issuer metadata only.
signature_with_a_spoofed_issuer_is_rejected(lines 120-150) states that issuer subpackets are attacker-controlled, so these assertions do not prove the pinned key signed anything.testdata/genesis-manifest-v1.3.2.txtandtestdata/genesis-manifest-v1.3.2.txt.ascare both committed, so one real check is available and it binds the embedded key to a production artifact.💚 Proposed additional assertion
fn published_manifest_parses_and_binds_its_assets() { + // The signed bytes are committed next to the signature, so verify the real thing. + verify_detached_signature( + include_bytes!("../testdata/genesis-manifest-v1.3.2.txt"), + include_bytes!("../testdata/genesis-manifest-v1.3.2.txt.asc"), + &ReleaseKey::fluent().expect("embedded key must load"), + ) + .expect("the published manifest signature must verify against the pinned key"); + let manifest = ReleaseManifest::parse(include_bytes!("../testdata/genesis-manifest-v1.3.2.txt")) .expect("published manifest must parse");🤖 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 `@crates/release-verify/src/tests.rs` around lines 580 - 610, Update published_signatures_are_issued_by_the_pinned_release_key to cryptographically verify at least one committed manifest/signature pair, such as testdata/genesis-manifest-v1.3.2.txt with its .asc signature, using the embedded ReleaseKey and the repository’s existing verification API. Retain the current metadata checks, but ensure the test asserts successful signature validation against the actual manifest bytes rather than trusting issuer subpackets.crates/release-verify/src/manifest.rs (1)
43-50: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject repeated
version=andcommit=lines, as the code already rejects conflicting digests.A repeated
version=line silently overwrites the earlier value, so the last line wins.checkusesself.versionto bind the artifact to a release, so this field carries the same "pick the convenient one" risk that motivates the digest-conflict guard on Line 68. The bytes are authenticated beforeparseruns, so this is a hardening gap and not an exploitable path.♻️ Proposed fix
if let Some(value) = line.strip_prefix("version=") { + if version.is_some() { + return Err(VerifyError::Manifest("multiple version= lines".to_owned())); + } version = Some(value.to_owned()); continue; } if let Some(value) = line.strip_prefix("commit=") { + if commit.is_some() { + return Err(VerifyError::Manifest("multiple commit= lines".to_owned())); + } commit = Some(value.to_owned()); continue; }🤖 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 `@crates/release-verify/src/manifest.rs` around lines 43 - 50, Update the manifest parsing logic around the version and commit assignments to reject duplicate version= and commit= entries instead of overwriting earlier values. Preserve the existing first-value behavior for unique entries and use the same conflict/error handling pattern already applied to repeated digest fields.bins/runtime-upgrade/provenance.rs (1)
171-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTwo copies of the bounded fetcher will drift.
fluentbase-release-verifydefines theFetchercontract but supplies no bounded HTTP implementation, so both consumers reimplement the same logic: thecontent_lengthpre-check,take(max_bytes + 1), and the post-read length check. Only the user-agent string and the error crate differ. A change to one copy, such as the HTTP 404 signal needed for the manifest-binding fix, will be missed in the other.
bins/runtime-upgrade/provenance.rs#L171-L197: move this implementation behind an optionalreqwestfeature offluentbase-release-verifyand call the shared fetcher with a configurable user-agent.crates/node/src/utils.rs#L54-L81: delete this copy and call the shared fetcher. Keepeyreconversion at the call site only.🤖 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 `@bins/runtime-upgrade/provenance.rs` around lines 171 - 197, Move the bounded HTTP logic from fetch_inner in bins/runtime-upgrade/provenance.rs#L171-L197 into fluentbase-release-verify behind an optional reqwest feature, exposing a shared fetcher that accepts a configurable user-agent; update fetch_inner to call it. Remove the duplicate implementation in crates/node/src/utils.rs#L54-L81 and call the shared fetcher there, retaining eyre conversion only at that call site.crates/node/src/chainspec.rs (1)
22-41: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd release CI for built-in genesis assets.
The tagged devnet/testnet/mainnet genesis
.json.gzSHA-256 pins match the published assets, but CI should also fetch the.ascfor each asset and assert both signature and digest. This keeps tag swaps from creating a silent startup mismatch for operators.🤖 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 `@crates/node/src/chainspec.rs` around lines 22 - 41, Add CI validation for the assets returned by devnet_genesis, testnet_genesis, and mainnet_genesis: for each configured release tag/channel, fetch the corresponding .json.gz and .asc files, verify the artifact signature, and assert its SHA-256 matches the pinned digest. Ensure the checks run for all three built-in genesis assets and fail CI on any fetch, signature, or digest mismatch.
🤖 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 `@bins/runtime-upgrade/main.rs`:
- Around line 92-109: Require an explicit genesis channel whenever the --rpc
argument is provided, so pick_rpc cannot use the default devnet artifact for a
custom endpoint. Update the rpc argument definition in CommonArgs with the
appropriate dependency on genesis_channel, while preserving the existing
--mainnet behavior and compatibility with tests that provide --genesis-channel.
In `@bins/runtime-upgrade/provenance.rs`:
- Around line 139-157: Extend FetchError with a not-found signal, set it in
fetch_inner for HTTP 404 and 410 responses, and update the load_verified error
handling in the provenance flow to downgrade only that case to
ManifestBinding::Unavailable. Propagate all other VerifyError::Fetch failures as
fatal errors, and add coverage confirming a non-404 manifest fetch failure
aborts the run.
In `@crates/codec/src/encoder.rs`:
- Around line 301-321: Update validate_collection_body to reject any non-zero
len when element_header_size is zero, returning the existing appropriate
decoding error before calculating required or attempting collection allocation;
preserve valid empty collections and all non-zero-header validation behavior.
In `@crates/codec/src/hash.rs`:
- Around line 585-586: In the tests module, remove the nested hashbrown import
of HashMap and HashSet while retaining the bytes import. Rely on the existing
use super::* to provide those types and eliminate the redundant-import warning.
In `@crates/release-verify/src/asset.rs`:
- Around line 33-57: Validate release asset identifiers before constructing
names or URLs: update ReleaseAsset::new, genesis, and manifest to reject tags,
channels, or resulting names containing unsafe characters or anything that is
not a single path component, using the AssetName error variant and propagating
Result through callers. Ensure externally supplied tags cannot introduce path
traversal or URL separators while preserving valid asset naming.
In `@crates/release-verify/src/key.rs`:
- Around line 44-61: Update ReleaseKey::new and signing_candidates to document
the release-key lifetime and revocation policy, and enforce it by rejecting
revoked or expired primary keys and signing-capable subkeys before they can be
used for verification. Ensure signing_candidates excludes any invalid components
while preserving valid candidates and the existing fingerprint and binding
checks.
In `@crates/release-verify/src/signature.rs`:
- Around line 99-108: Update signing_candidates to exclude revoked or expired
primary keys, user/packet contexts, and signing subkeys before creating
SigningCandidate values; do not rely solely on key_flags().sign(). Apply the
existing OpenPGP revocation and temporal-policy checks, including rejecting
future issuance timestamps and expired signature validity windows, while
preserving valid signing candidates.
---
Nitpick comments:
In `@bins/runtime-upgrade/provenance.rs`:
- Around line 171-197: Move the bounded HTTP logic from fetch_inner in
bins/runtime-upgrade/provenance.rs#L171-L197 into fluentbase-release-verify
behind an optional reqwest feature, exposing a shared fetcher that accepts a
configurable user-agent; update fetch_inner to call it. Remove the duplicate
implementation in crates/node/src/utils.rs#L54-L81 and call the shared fetcher
there, retaining eyre conversion only at that call site.
In `@crates/codec/src/encoder.rs`:
- Around line 262-299: Clarify in the documentation for checked_decode_slice and
checked_decode_slice_from that they require the requested range to be available
in the current contiguous buf.chunk(), or update both helpers to validate bounds
against buf.remaining() consistently with nearby decode helpers before slicing
chunk. Preserve the existing overflow and BufferTooSmall error behavior.
In `@crates/codec/src/hash.rs`:
- Around line 588-622: Add regression tests covering the Solidity collection
decoders for both HashMap and HashSet, alongside the existing compact tests.
Build fixtures with an oversized claimed count and headers that exercise the
checked_decode_slice_from path in the Solidity implementations, then assert
decoding fails with BufferTooSmall or Overflow before allocation. Use the
existing Solidity decoder test patterns and retain separate tests for each
collection type.
In `@crates/codec/src/vec.rs`:
- Around line 266-296: Extend both compact and Solidity vector decoder tests to
cover the off-by-one boundary where the declared element count is exactly one
greater than the available body capacity. Keep the existing empty-body and error
assertions, and add fixtures/assertions that confirm this count is rejected
before allocation.
In `@crates/node/src/chainspec.rs`:
- Around line 22-41: Add CI validation for the assets returned by
devnet_genesis, testnet_genesis, and mainnet_genesis: for each configured
release tag/channel, fetch the corresponding .json.gz and .asc files, verify the
artifact signature, and assert its SHA-256 matches the pinned digest. Ensure the
checks run for all three built-in genesis assets and fail CI on any fetch,
signature, or digest mismatch.
In `@crates/release-verify/src/load.rs`:
- Around line 205-224: Update write_atomic to create a process-specific
temporary path in the same directory, incorporating the current process id into
the existing “.tmp” name or using tempfile::NamedTempFile with persist. Ensure
all create, write, sync, and rename operations use this unique path, preserving
the existing atomic replacement and error reporting behavior.
In `@crates/release-verify/src/manifest.rs`:
- Around line 43-50: Update the manifest parsing logic around the version and
commit assignments to reject duplicate version= and commit= entries instead of
overwriting earlier values. Preserve the existing first-value behavior for
unique entries and use the same conflict/error handling pattern already applied
to repeated digest fields.
In `@crates/release-verify/src/tests.rs`:
- Around line 580-610: Update
published_signatures_are_issued_by_the_pinned_release_key to cryptographically
verify at least one committed manifest/signature pair, such as
testdata/genesis-manifest-v1.3.2.txt with its .asc signature, using the embedded
ReleaseKey and the repository’s existing verification API. Retain the current
metadata checks, but ensure the test asserts successful signature validation
against the actual manifest bytes rather than trusting issuer subpackets.
🪄 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: e0239a76-2213-48af-9edb-56bff83558c9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomlbins/runtime-upgrade/Cargo.tomlbins/runtime-upgrade/main.rsbins/runtime-upgrade/provenance.rscontracts/runtime-upgrade/src/tests.rscrates/codec/src/bytes_codec.rscrates/codec/src/encoder.rscrates/codec/src/hash.rscrates/codec/src/vec.rscrates/node/Cargo.tomlcrates/node/src/chainspec.rscrates/node/src/utils.rscrates/release-verify/Cargo.tomlcrates/release-verify/src/asset.rscrates/release-verify/src/error.rscrates/release-verify/src/key.rscrates/release-verify/src/lib.rscrates/release-verify/src/load.rscrates/release-verify/src/manifest.rscrates/release-verify/src/signature.rscrates/release-verify/src/test_support.rscrates/release-verify/src/tests.rscrates/release-verify/testdata/genesis-mainnet-v1.0.0.json.gz.asccrates/release-verify/testdata/genesis-manifest-v1.3.2.txtcrates/release-verify/testdata/genesis-manifest-v1.3.2.txt.asccrates/release-verify/testdata/genesis-v0.3.4-dev.json.gz.asccrates/release-verify/testdata/genesis-v0.5.7.json.gz.asc
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/runtime/src/runtime/contract_runtime.rs (2)
159-170: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
memory_pagescan panic throughexpecton a consensus path.
u32::try_from(...).expect(...)aborts the node if the page count ever exceedsu32::MAX. The value is bounded by the configured page budget today, so the panic is unreachable. Consider returning a saturating value instead, so a future change to the memory model cannot turn an accounting detail into a node crash.♻️ Proposed defensive change
- u32::try_from(store.memory_size_bytes() / page_bytes) - .expect("rwasm: memory page count must fit u32") + u32::try_from(store.memory_size_bytes() / page_bytes).unwrap_or(u32::MAX)🤖 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 `@crates/runtime/src/runtime/contract_runtime.rs` around lines 159 - 170, Update ContractRuntime::memory_pages to avoid panicking when converting the computed rWasm page count to u32. Replace the expect-based conversion with saturating behavior that returns u32::MAX on overflow, while preserving the existing page calculation and Wasmtime return value.
258-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a rejected initializer shape.
The tests cover fuel metering, underfunding, and budget rejection. They do not cover the
IllegalOpcodepaths ininitial_memory_pages, for example twoMemoryGrowinstructions in the initializer, aMemoryGrowthat is not followed byDrop, or aMemoryGrowat index 0. Those branches guard pre-allocation metering, so they deserve direct coverage.🤖 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 `@crates/runtime/src/runtime/contract_runtime.rs` around lines 258 - 305, Add focused tests for the IllegalOpcode branches in initial_memory_pages, using modules whose memory initializers contain two MemoryGrow instructions, a MemoryGrow not followed by Drop, and a MemoryGrow at index 0. Invoke ContractRuntime::new with each module and assert that initialization is rejected with TrapCode::IllegalOpcode before allocation.crates/runtime/src/executor.rs (1)
556-572: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
module_with_initial_memorytest fixture in two modules. Both test modules define the same rWasm module builder for initial-memory coverage. One shared fixture prevents the two copies from drifting from the initializer shape thatinitial_memory_pagesaccepts.
crates/runtime/src/executor.rs#L556-L572: remove the local helper and import the shared fixture.crates/runtime/src/runtime/contract_runtime.rs#L233-L249: move this helper into a shared#[cfg(test)]support module and re-export it for both test modules.🤖 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 `@crates/runtime/src/executor.rs` around lines 556 - 572, The duplicated module_with_initial_memory test fixture should be centralized. In crates/runtime/src/runtime/contract_runtime.rs#L233-L249, move the helper into a shared #[cfg(test)] support module and re-export it; in crates/runtime/src/executor.rs#L556-L572, remove the local module_with_initial_memory definition and import the shared fixture, preserving the existing initializer behavior.contracts/runtime-upgrade/src/tests.rs (1)
118-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
UpgradePlanCancelledpayload in the cancellation tests.
assert_plan_is_cancelledverifies storage state only. The tests check theUpgradePlanCancelledsignature at Line 288 and check the absence of the event at Line 428, but no test checks that a cancellation emits the event with the planned targets and hashes. Add a log assertion inassert_plan_is_cancelledto cover the emitted payload.♻️ Suggested addition
fn assert_plan_is_cancelled(h: &mut Harness, hash_a: B256, hash_b: B256) { + let logs = h.sdk.take_logs(); + assert!( + logs.iter() + .any(|log| log.1[0].0 == UpgradePlanCancelled::SELECTOR), + "UpgradePlanCancelled was not emitted" + ); assert_eq!( h.planned_updater(), Address::ZERO, "updater still delegated" );Note that
Harness::calldoes not clear logs between calls, so the helper must tolerate earlierUpgradePlannedandOwnerChangedentries.🤖 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 `@contracts/runtime-upgrade/src/tests.rs` around lines 118 - 146, Update assert_plan_is_cancelled to assert that cancellation emitted an UpgradePlanCancelled event containing TARGET_A/hash_a and TARGET_B/hash_b. Search accumulated logs without assuming Harness::call clears them, and tolerate earlier UpgradePlanned and OwnerChanged entries while verifying the expected cancellation payload.
🤖 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 `@contracts/runtime-upgrade/src/lib.rs`:
- Around line 347-380: Use the remaining target/hash pair count as the sole
plan-existence check in cancel_planned_upgrade, so it returns without emitting
when no pairs remain. Update remove_planned_upgrade to clear planned_updater and
planned_genesis metadata when it removes the final pair, and add a regression
test covering final-pair execution followed by ownership change with no
UpgradePlanCancelled event.
In `@e2e/src/lib.rs`:
- Around line 41-42: Move the #[cfg(test)] attribute directly before the mod
eip7951; declaration so the EIP-7951 module is compiled only in test builds;
remove the misplaced standalone attribute.
---
Nitpick comments:
In `@contracts/runtime-upgrade/src/tests.rs`:
- Around line 118-146: Update assert_plan_is_cancelled to assert that
cancellation emitted an UpgradePlanCancelled event containing TARGET_A/hash_a
and TARGET_B/hash_b. Search accumulated logs without assuming Harness::call
clears them, and tolerate earlier UpgradePlanned and OwnerChanged entries while
verifying the expected cancellation payload.
In `@crates/runtime/src/executor.rs`:
- Around line 556-572: The duplicated module_with_initial_memory test fixture
should be centralized. In
crates/runtime/src/runtime/contract_runtime.rs#L233-L249, move the helper into a
shared #[cfg(test)] support module and re-export it; in
crates/runtime/src/executor.rs#L556-L572, remove the local
module_with_initial_memory definition and import the shared fixture, preserving
the existing initializer behavior.
In `@crates/runtime/src/runtime/contract_runtime.rs`:
- Around line 159-170: Update ContractRuntime::memory_pages to avoid panicking
when converting the computed rWasm page count to u32. Replace the expect-based
conversion with saturating behavior that returns u32::MAX on overflow, while
preserving the existing page calculation and Wasmtime return value.
- Around line 258-305: Add focused tests for the IllegalOpcode branches in
initial_memory_pages, using modules whose memory initializers contain two
MemoryGrow instructions, a MemoryGrow not followed by Drop, and a MemoryGrow at
index 0. Invoke ContractRuntime::new with each module and assert that
initialization is rejected with TrapCode::IllegalOpcode before allocation.
🪄 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: 0ffb9823-6a40-426a-8b73-f8b43133ee69
📒 Files selected for processing (9)
contracts/eip7951/src/lib.rscontracts/runtime-upgrade/README.mdcontracts/runtime-upgrade/src/lib.rscontracts/runtime-upgrade/src/tests.rscrates/runtime/src/executor.rscrates/runtime/src/runtime.rscrates/runtime/src/runtime/contract_runtime.rse2e/src/eip7951.rse2e/src/lib.rs
| mod eip7951; | ||
| #[cfg(test)] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Guard the EIP-7951 test module with #[cfg(test)].
#[cfg(test)] on Line 42 does not apply to mod eip7951; on Line 41. Move the attribute before the module declaration. This prevents test-only code from compiling in non-test builds.
Proposed fix
-mod eip7951;
#[cfg(test)]
+mod eip7951;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mod eip7951; | |
| #[cfg(test)] | |
| #[cfg(test)] | |
| mod eip7951; |
🤖 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 `@e2e/src/lib.rs` around lines 41 - 42, Move the #[cfg(test)] attribute
directly before the mod eip7951; declaration so the EIP-7951 module is compiled
only in test builds; remove the misplaced standalone attribute.
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
b021c63 to
7798f2e
Compare
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
7798f2e to
9944bf2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/runtime/src/runtime/contract_runtime.rs (4)
16-17: 📐 Maintainability & Code Quality | 🔵 TrivialRun the required Rust checks before merge.
Run
cargo fmt --checkfor the touched Rust code. Run Clippy with-D warnings. Confirm that the declaring Cargo manifest uses Rust edition 2021 and that the runtime path remainsno_std.As per coding guidelines, Rust changes must pass
cargo fmt --check, Clippy warnings are errors in CI with-D warnings, and runtime paths must preserveno_stdconstraints.🤖 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 `@crates/runtime/src/runtime/contract_runtime.rs` around lines 16 - 17, Before finalizing the Rust change, run cargo fmt --check and Clippy with -D warnings for the touched code. Verify the declaring Cargo manifest uses Rust edition 2021, and ensure the runtime code around contract runtime imports preserves its no_std compatibility.Source: Coding guidelines
215-227: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftMake the charge-before-allocation guarantee observable.
The funded test checks only the final fuel amount. The underfunded test checks only
OutOfFuel. An implementation that allocates memory first and charges fuel afterward could produce the same results when allocation succeeds.If the requirement is to charge fuel before instantiation, add an allocation trace, hook, or other observable assertion that proves
ConsumeFuelprecedesMemoryGrow. Otherwise, narrow the test names and comments to fuel accounting only.Also applies to: 229-243
🤖 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 `@crates/runtime/src/runtime/contract_runtime.rs` around lines 215 - 227, Update the tests meters_initial_memory_before_instantiation and the underfunded counterpart to observe ordering, not just fuel totals or OutOfFuel: add an allocation trace, hook, or equivalent assertion proving ConsumeFuel occurs before MemoryGrow during ContractRuntime::new. If no observable ordering mechanism is available, rename and revise the tests/comments to describe only fuel accounting.
173-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test name and failure text with the new behavior.
rwasm_initializer_allocates_maximum_memory_without_a_fuel_chargeasserts that the initializer consumes fuel. The comment andexpectmessage still describe an unmetered initializer. Rename the test and update the stale text.Proposed fix
-fn rwasm_initializer_allocates_maximum_memory_without_a_fuel_charge() { +fn rwasm_initializer_charges_initial_memory_fuel() { ... - // compiler-generated initializer. It must fail with OutOfFuel once that initializer - // emits a proportional ConsumeFuel before MemoryGrow. + // The compiler-generated initializer must consume a proportional + // ConsumeFuel charge before MemoryGrow. ... - .expect("the current unmetered initializer allocates before checking fuel"); + .expect("sufficient fuel must cover the initial-memory charge");🤖 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 `@crates/runtime/src/runtime/contract_runtime.rs` around lines 173 - 213, The test name rwasm_initializer_allocates_maximum_memory_without_a_fuel_charge contradicts its fuel-consumption assertions. Rename it to describe the metered initializer behavior, and update the stale executor.expect message and nearby comments to state that allocation consumes fuel through the bulk-operation charge.
135-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove
initial_memory_fuelunder the test module when moving runtime charge handling.
initial_memory_fueland its rWasm imports are only called inside#[cfg(test)], so the productionContractRuntime::newpath does not use thechecked_muloverflow handling and this helper can trigger dead-code warnings with-D warnings. Gate the helper in#[cfg(test)]unless the runtime constructor is updated to charge initial memory fuel before callingcreate_executor.🤖 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 `@crates/runtime/src/runtime/contract_runtime.rs` around lines 135 - 141, Move the initial_memory_fuel helper and its rWasm-related imports into the #[cfg(test)] module, since they are only used by tests. Alternatively, update ContractRuntime::new to charge initial memory fuel before create_executor, but preserve the existing overflow handling; prefer test-only gating if no production runtime behavior is required.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 `@bins/runtime-upgrade/main.rs`:
- Around line 708-718: After loading the release in the provenance-gated startup
flow, obtain the RPC chain ID and compare it with genesis.config.chain_id before
entering any plan or transaction logic. Reject mismatches, including explicitly
selected --genesis-channel artifacts against custom RPC endpoints, and add a
regression test covering that case.
---
Nitpick comments:
In `@crates/runtime/src/runtime/contract_runtime.rs`:
- Around line 16-17: Before finalizing the Rust change, run cargo fmt --check
and Clippy with -D warnings for the touched code. Verify the declaring Cargo
manifest uses Rust edition 2021, and ensure the runtime code around contract
runtime imports preserves its no_std compatibility.
- Around line 215-227: Update the tests
meters_initial_memory_before_instantiation and the underfunded counterpart to
observe ordering, not just fuel totals or OutOfFuel: add an allocation trace,
hook, or equivalent assertion proving ConsumeFuel occurs before MemoryGrow
during ContractRuntime::new. If no observable ordering mechanism is available,
rename and revise the tests/comments to describe only fuel accounting.
- Around line 173-213: The test name
rwasm_initializer_allocates_maximum_memory_without_a_fuel_charge contradicts its
fuel-consumption assertions. Rename it to describe the metered initializer
behavior, and update the stale executor.expect message and nearby comments to
state that allocation consumes fuel through the bulk-operation charge.
- Around line 135-141: Move the initial_memory_fuel helper and its rWasm-related
imports into the #[cfg(test)] module, since they are only used by tests.
Alternatively, update ContractRuntime::new to charge initial memory fuel before
create_executor, but preserve the existing overflow handling; prefer test-only
gating if no production runtime behavior is required.
🪄 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: b5f63ed5-9a9d-43dd-b8ba-ec5b88ac7602
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.gitignoreCargo.tomlbins/runtime-upgrade/Cargo.tomlbins/runtime-upgrade/main.rsbins/runtime-upgrade/provenance.rscontracts/eip7951/src/lib.rscontracts/fee-manager/src/lib.rscontracts/runtime-upgrade/README.mdcontracts/runtime-upgrade/src/lib.rscontracts/runtime-upgrade/src/tests.rscrates/codec/src/bytes_codec.rscrates/codec/src/encoder.rscrates/codec/src/hash.rscrates/codec/src/vec.rscrates/node/Cargo.tomlcrates/node/src/chainspec.rscrates/node/src/utils.rscrates/release-verify/Cargo.tomlcrates/release-verify/src/asset.rscrates/release-verify/src/error.rscrates/release-verify/src/key.rscrates/release-verify/src/lib.rscrates/release-verify/src/load.rscrates/release-verify/src/manifest.rscrates/release-verify/src/signature.rscrates/release-verify/src/test_support.rscrates/release-verify/src/tests.rscrates/release-verify/testdata/genesis-mainnet-v1.0.0.json.gz.asccrates/release-verify/testdata/genesis-manifest-v1.3.2.txtcrates/release-verify/testdata/genesis-manifest-v1.3.2.txt.asccrates/release-verify/testdata/genesis-v0.3.4-dev.json.gz.asccrates/release-verify/testdata/genesis-v0.5.7.json.gz.asccrates/runtime/src/runtime/contract_runtime.rse2e/src/builtins.rse2e/src/ddos.rse2e/src/eip7951.rse2e/src/fee_manager.rse2e/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (32)
- crates/node/Cargo.toml
- crates/release-verify/testdata/genesis-manifest-v1.3.2.txt.asc
- crates/release-verify/testdata/genesis-mainnet-v1.0.0.json.gz.asc
- crates/release-verify/testdata/genesis-v0.3.4-dev.json.gz.asc
- crates/release-verify/testdata/genesis-v0.5.7.json.gz.asc
- bins/runtime-upgrade/Cargo.toml
- crates/codec/src/encoder.rs
- e2e/src/eip7951.rs
- contracts/runtime-upgrade/README.md
- crates/node/src/chainspec.rs
- e2e/src/lib.rs
- crates/release-verify/Cargo.toml
- crates/release-verify/src/lib.rs
- .gitignore
- e2e/src/ddos.rs
- contracts/eip7951/src/lib.rs
- Cargo.toml
- contracts/runtime-upgrade/src/lib.rs
- crates/release-verify/src/tests.rs
- crates/release-verify/src/key.rs
- crates/release-verify/src/manifest.rs
- crates/codec/src/hash.rs
- crates/release-verify/src/test_support.rs
- crates/release-verify/src/signature.rs
- crates/release-verify/src/load.rs
- crates/node/src/utils.rs
- crates/codec/src/vec.rs
- contracts/runtime-upgrade/src/tests.rs
- crates/codec/src/bytes_codec.rs
- crates/release-verify/testdata/genesis-manifest-v1.3.2.txt
- bins/runtime-upgrade/provenance.rs
- crates/release-verify/src/asset.rs
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
dmitry123
changed the title
dmitry123
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/runtime/src/executor.rs`:
- Around line 408-430: The memory limit is checked before execution but not when
an interrupted runtime is retained, allowing execute or resume to grow memory
past the aggregate limit. Update try_remember_runtime to calculate the current
in-flight memory including the runtime being admitted and reject it before
inserting into recoverable_runtimes; preserve the existing rejection
result/cleanup behavior, and add a regression test that grows memory before
interruption.
In `@crates/sdk/src/types/storage.rs`:
- Around line 90-137: Extend the short-string tests around
write_storage_short_string and storage_short_string to cover the non-canonical
value "USDC\0fake": assert the write returns ExitCode::MalformedBuiltinParams
without changing storage, and seed the same bytes through raw storage then
assert the read returns that error.
In `@crates/sdk/src/universal_token/storage.rs`:
- Around line 392-435: Extend the token-name regression tests around
TokenNameOrSymbol::try_from_str and TokenNameOrSymbol::from_word with
non-canonical metadata cases: assert that "USDC\0fake" is rejected and that a
raw B256 word containing nonzero bytes after its first NUL is rejected during
decoding. Verify the assertions cover both encoding and decoding paths without
changing existing valid-name behavior.
- Around line 40-73: Enforce canonical NUL-terminated short strings across all
affected sites: in crates/sdk/src/universal_token/storage.rs lines 40-73, reject
embedded NULs in text constructors and reject nonzero bytes after the first NUL
during decoding; in crates/sdk/src/universal_token.rs lines 182-194, make
try_build use the canonical validator and ensure build cannot bypass it; in
crates/sdk/src/types/storage.rs lines 22-40, validate canonical form on both
reads and writes; in contracts/universal-token/src/lib.rs lines 720-729, return
ExitCode::MalformedBuiltinParams before either storage write; and in
contracts/universal-token/src/tests.rs lines 1086-1155, add constructor and
stored-slot regressions for USDC\0fake.
🪄 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: 8bb2794c-4d90-4975-a31c-1aa29300c91a
📒 Files selected for processing (9)
contracts/universal-token/src/lib.rscontracts/universal-token/src/tests.rscrates/runtime/src/executor.rscrates/runtime/src/runtime.rscrates/runtime/src/runtime/contract_runtime.rscrates/sdk/src/types/storage.rscrates/sdk/src/universal_token.rscrates/sdk/src/universal_token/storage.rscrates/types/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/runtime/src/runtime/contract_runtime.rs
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
…ersisting metadata Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
…g on directory order
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
604cc4a to
6fcfbae
Compare
6fcfbae to
2b91e95
Compare
…t alias earlier elements
…d of matching caller-selected substrings
…truncated u64/i64 buffers error instead of panicking
…o blob transactions
…ts and keep value off non-payable ones
…s indexed-parameter rules instead of ordinary ABI encoding
…o routers and published ABIs agree
2 participants
Summary
Fixes from the August 2026 security audit: seven independent issues across the runtime, codec, node genesis path, and system contracts, plus regression coverage for an eighth that is reproduced but not fixed here.
42 files, +4343/-292, 11 self-contained commits. Sections below are ordered by blast radius.
Release artifact authentication
New
fluentbase-release-verifycrate. Genesis allocations, system contract code, and runtime-upgrade payloads are all derived from GitHub release assets, so they are only as trustworthy as the signature check applied to them. The crate treats every artifact as untrusted until authenticated against a pinned release key, and is deliberately fail-closed:bins/runtime-upgradeauthenticates release artifacts before signing, and reports provenance.Fail-closed genesis verification (node)
Built-in networks now resolve genesis through
download_and_cache_genesis_verifiedagainst pinned SHA-256 digests plus a detached OpenPGP signature. Mainnet and the genesis channel became explicit, validated selections. The network table is kept in one place so tests can assert the fail-closed guarantees hold for every entry.Nested-frame memory exhaustion (runtime)
A suspended frame keeps its whole store — linear memory included — alive in
recoverable_runtimesuntil resumed or forgotten, so a call chain holdsdepth × frame_sizebytes resident simultaneously. Per-frame fuel prices a single allocation but cannot bound the sum across frames:CALL_STACK_LIMITframesMAX_IN_FLIGHT_MEMORY_BYTES(1.5 GiB) now bounds the memory held by all live frames of a transaction; a frame pushing the total past it is rejected withExitCode::OutOfMemory. Measured end to end: a chain demanding 63.94 GiB is admitted 24 frames and holds 1.50 GiB.Two implementation notes:
recoverable_runtimeson demand rather than tracked incrementally, so it cannot drift from the frames actually alive.RwasmModule— it is encoded in entrypoint bytecode — so checking beforehand would mean decoding that prologue and coupling the runtime to rWasm codegen. Measuring the constructed frame avoids that; overshoot is bounded by one frame.System runtimes are excluded: their store is cached and reused across calls rather than allocated per frame, so counting it per frame would multiply-count one allocation.
No gas costs change. Behaviour changes only for transactions exceeding the cap. The limit sits above the worst case ordinary contracts can reach (full depth at the toolchain-default 17 initial pages, ~1.06 GiB), so nothing that succeeds today begins to fail — but it is consensus-visible and needs a coordinated upgrade.
Decoder allocation guards (codec)
Collection decoding validated element counts against the remaining buffer before allocating. Header offsets and element offsets are now computed with checked arithmetic, bodies are validated via
validate_collection_bodybefore reserving, and capacity is obtained withtry_reserveso a malformed length cannot abort on OOM.System contract ownership
runtime-upgrade— delegated upgrade authority could outlive the owner that granted it. Any ownership transition (change_owner,renounce_ownership) now atomically cancels a pending plan — updater, genesis metadata, and every remaining target/hash pair — emittingUpgradePlanCancelled. A new owner must re-authorize. No-op when nothing is planned, so transitions stay cheap and emit no misleading event.fee-manager— zero is not a neutral owner value:owner()andonly_owner()map an empty slot back to the genesis bootstrap key, so a zero transfer would silently reactivate a retired authority after governance had handed control over. Zero-address transfers are now rejected;renounceOwnershipremains the explicit fork-only transition.EIP-7951 gas accounting
The precompile wrapper moved to
p256_verify_osaka/P256VERIFY_BASE_GAS_FEE_OSAKA, correcting both the charged gas and the documented return shape (32 bytes ending in 1, not a single byte).Reproduced, not fixed: aggregate output growth
e2e/src/ddos.rscharacterizes a separate, still-open issue. Fuel is charged per_writebut does not bound the aggregate host output buffer: a contract reusing 1 MiB of guest memory appends 64 MiB of output for under 7M gas. Scaled to a 100M gas block that permits over 900 MiB of output.This is orthogonal to the frame-memory cap above — that bounds linear memory across frames, this is a single frame growing the host-side output
Vec. The test asserts the current behaviour so the vector is pinned and any future fix has a baseline; it is not a fix.Testing
fluentbase-release-verify— signature, manifest, asset, key, and load coverage with signed testdata fixtures.crates/runtime— in-flight accounting, admission and rejection at the cap, and a full 1024-iteration call chain proving depth alone never trips the limit while memory-heavy chains are cut off early.contracts/runtime-upgrade,e2e/fee_manager.rs— ownership transitions cancel pending plans; zero-address transfer rejected.e2e/eip7951.rs,e2e/builtins.rs— Osaka gas figures; builtin expectations updated for the initial-memory charge introduced by the rWasm v0.4.6 upgrade.recursion_demanding_64_gib_is_capped_at_the_in_flight_limitis#[ignore]d — it allocates ~1.5 GiB resident by design, since proving the cap holds means allocating up to it. Run explicitly:cargo test -p fluentbase-runtime --lib recursion_demanding -- --ignored --nocaptureSummary by CodeRabbit
New Features
Bug Fixes