fix(symbolic): find writes beyond materialized_size in dynamic memory reads - #16655
Open
gomesalexandre wants to merge 1 commit into
Open
fix(symbolic): find writes beyond materialized_size in dynamic memory reads#16655gomesalexandre wants to merge 1 commit into
gomesalexandre wants to merge 1 commit into
Conversation
… reads byte_dynamic_with_delta (used by the dynamic-offset paths of MLOAD and memory-copy opcodes) enumerated candidate positions only within 0..materialized_size when resolving a symbolic read offset. A write recorded via store_symbolic_bytes never bumps materialized_size -- there is no single concrete candidate to extend it to for a write whose own offset isn't (or only looks) symbolic -- so such a write could land at a position the enumeration never reaches, silently returning zero instead of its real value. In a soundness-sensitive symbolic executor, that false negative can hide a genuine violation behind an apparent "Safe" result. Fix: fall back to folding every recorded write directly against the (symbolic) read target -- mirroring byte()'s own technique for a symbolic write offset against a concrete read target, generalized to a symbolic one -- whenever any write's full extent isn't provably within materialized_size. The bound check is deliberately the write's full extent (offset + length <= materialized_size, checked arithmetic), not just whether its offset is const-evaluable: materialized_size is only ever bumped based on the store dispatch's shallow offset.as_const() check, which is strictly narrower than the .eval() used elsewhere to decide whether an offset is "concrete" -- a write can be dispatched as symbolic (no bump) while still reporting as concrete under the wider check, silently invalidating the bound. load_word_offset now delegates its dynamic-offset branch through the same fixed read path instead of the separately-buggy load_word_dynamic (deleted), which had the identical bound-enumeration defect for full 32-byte word reads. Bumped by PR foundry-rs#16240, which introduced the materialized_size / logical_size split specifically to avoid false proofs; this closes the one write path (store_symbolic_bytes) that split didn't yet cover. Adds regression tests reproducing the false negative directly (a write whose offset is const-evaluable but not const-foldable at construction time, via Keccak(const) & mask -- so it's dispatched as symbolic without ever being caught by a naive "is this write's offset concrete" check), a write straddling the materialized-size boundary, and the exact byte-beyond-bound / dynamic-word-read-at-a-different-offset shape that the deleted load_word_dynamic got wrong. No functional change to any other read/write path; SymBytes and storage already used the sound fold-every-write technique and are unaffected.
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
gomesalexandre
marked this pull request as ready for review
September 5, 2026 00:34
gomesalexandre
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr,
mattsse and
stevencartavia
as code owners
September 5, 2026 00:34
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
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.
Summary
byte_dynamic_with_delta(the dynamic-offset read path used by MLOAD and memory-copy opcodes when the read offset is symbolic) enumerated candidate positions only within0..materialized_size.store_symbolic_bytesrecords a write without ever bumpingmaterialized_size-- there's no single concrete candidate to extend it to for a write whose offset the store dispatch treated as symbolic -- so such a write could land at a position the enumeration never reaches, and the read silently returned zero instead of the write's real value.In a soundness-sensitive symbolic execution engine, a false negative like this can hide a genuine violation behind an apparently-"Safe" result.
This closes the one write path that #16240 (which introduced the
materialized_size/logical_sizesplit, explicitly to avoid false proofs) didn't yet cover.Root cause
Store dispatch (
store_word_offset/store_byte_offset/store_bytes_offset) decides whether a write's offset is "concrete" usingoffset.as_const()-- a shallow check that only matches a literalConstnode.SymbolicMemoryWrite::concrete_offset()instead usesoffset.eval()-- full recursive evaluation, strictly broader. A write whose offset happens to be const-evaluable but never folds down to a literalConstat construction time (e.g.Keccak(constant_preimage) & 0xff, since the mask-folding rules have no case for a bareKeccakoperand) gets dispatched throughstore_symbolic_bytesand never bumpsmaterialized_size, whileconcrete_offset()still reports it as "concrete."A naive fast-path predicate that trusts
concrete_offset().is_some()alone would wrongly treatmaterialized_sizeas a valid bound for such a write.Fix
byte_dynamic_with_deltanow only takes the bounded-enumeration fast path when it can prove, for every recorded write, that the write's full extent -- not just its offset -- fits insidematerialized_size(offset.checked_add(len)someend <= materialized_size, checked arithmetic). Otherwise it falls back to folding every recorded write directly against the symbolic read target via nestedite, mirroringbyte()'s own already-sound technique for a symbolic write offset against a concrete read target, generalized to a symbolic one.load_word_offset's dynamic-offset branch now delegates through this same fixed path instead of the separately-buggyload_word_dynamic(deleted), which had the identical defect for full 32-byte word reads -- plus its own byte-by-byte-vs-whole-word enumeration mismatch.No other read/write path is touched.
SymBytes(byte_dynamic_with_deltainbytes.rs-- different struct, same name) and the storage layer (StorageWrite::select_from) already use the sound fold-every-write technique unconditionally, which is what confirms the approach here matches established convention rather than introducing a new one.Testing
Keccak(const) & 0xffbut notas_const()-able (dispatched as symbolic,materialized_sizenever bumped), a write straddling thematerialized_sizeboundary (starts inside, ends past it), and the exact byte-beyond-bound / word-read-starting-at-a-different-offset shape the deletedload_word_dynamicgot wrong.concrete_offset().is_some()predicate reproduces the exact silent-zero failure on the new tests; restoring the fix makes them pass.cargo clippy -p foundry-evm-symbolic --lib -- -D warnings: clean.cargo fmt --check -p foundry-evm-symbolic: clean, no diff.Disclosure note
This defect sits in the symbolic execution engine's soundness guarantees (a false negative in a security-analysis tool). No separate private disclosure was made since it requires no live/deployed target to exploit -- it's a correctness bug in the local analysis tool itself, not a vulnerability in code the tool analyzes.