fix(storage): cleanup tail in dyn types#3840
Conversation
0xrusowsky
force-pushed
the
rus/clear-dyn-writes
branch
from
0b5e41b to
33254c0
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 5647/7748 lines (72.88%) File details
contractsCoverage: 1/253 lines (0.40%) File details
Total: 5648/8001 lines (70.59%) |
0xrusowsky
requested review from
0xKitsune,
fgimenez,
klkvr,
legion2002 and
mattsse
as code owners
| // Handlers always use `FULL` ctx: | ||
| // `T::Handler::write(v)` → `self.as_slot().write(v)` → `Slot::<T>::new(s, a).write(v)`. | ||
| // Since the slot we push to is guaranteed empty, we build the `Slot<T>` directly to | ||
| // thread `INIT` into `T::store` and skip its tail-cleanup SLOAD for dynamic types. | ||
| let elem_slot = self.data_slot() + U256::from(length * T::SLOTS); | ||
| Slot::<T>::new_with_ctx(elem_slot, LayoutCtx::INIT, self.address).write(value)?; |
There was a problem hiding this comment.
NOTE: i believe this workaround is safe because of the reasons explained in the cmnt
in order to use Self::compute_handler with LayoutCtx::INIT we'd have to change all non-primitive handlers to use and hold ctx (which currently ignore)
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Four worker passes (w1×3, w2×2) returned NO_FINDING for any T2-active vulnerability. The PR is well-scoped and the new T5-gated cleanup branches plus LayoutCtx::INIT are deterministic, bounded, and consistent with the typed-storage invariants.
Head drift note: workers audited 0b5e41b40efb; this review targets bcc60e304e3fcdad7818f75618fe71019cde6104. The drift moved gating from T4 to T5 and fixed the previously-broken test compile (ONE → U256::ONE in structs.rs:309), so that finding has been dropped as stale.
🛡️ [DEFENSE-IN-DEPTH] [T; N] Storable impl not migrated to is_full()
File: crates/precompiles-macros/src/storable_primitives.rs:378,387,399 (not in this PR's diff — body-only)
Sister types (Vec, String, Bytes, struct macro) were uniformly updated to debug_assert!(ctx.is_full(), …), but gen_array_impl's generated handle/load/store still use debug_assert_eq!(ctx, LayoutCtx::FULL, …). Today this is dormant — no production Vec<[T; N]> exists and the struct macro forces FULL on non-dynamic fields. But if anyone adds Vec<[u8; 32]> (or any [T; N] with T::BYTES * N > 16), Vec::push will thread INIT into [T; N]::store and the equality assert will fire in debug/test builds (release silently works because store_impl doesn't consult ctx).
Recommended Fix: Replace the three debug_assert_eq!(ctx, LayoutCtx::FULL, …) calls with debug_assert!(ctx.is_full(), …) for symmetry.
See inline comments for the other two items.
Reviewer Callouts
-
⚡ Gas-cost change for shrinking dynamic types under T5 —
Vec::storeandstore_bytes_likeadd up toprev - newextra SSTOREs (orT::deletecalls for unpacked) on T5. Gas-metered and not griefable, but per-operation costs of shrinking change post-T5. Worth confirming downstream precompile gas budgets (e.g.,account_keychainrevoke flows,stablecoin_dexbook mutations, validator config rewrites with long string fields) accommodate the additional SSTOREs. -
⚡
LayoutCtx::INIT"virgin by construction" forVec::pushis fork-history sensitive — Holds for anyVecwhose entire lifetime is on T5 (every shrinking path zeros freed elements). Does NOT hold for aVecwhose history includes a pre-T5 shrink. Concretely,write(vec![long_a, long_b, long_c])thenwrite(vec![x, y])under T2/T3/T4 leaveslong_c's chunks live; a later T5push("z")to index 2 writes the base slot withINITbut the legacy chunks atkeccak256(data_start + 2) + ipersist. Impact is purely storage bloat — every read path is length-bounded andkeccak256precludes collisions with live storage. -
⚡ Pre-T5 → T5 activation hygiene — Combined with the bytes_like inline finding, a human reviewer should confirm the T5 activation plan accounts for legacy dynamic tails that may already exist when T5 activates. Cleanup only runs on subsequent writes/deletes; pre-existing ghost data in long validator config strings, TIP-20 metadata, etc. is never proactively scrubbed.
-
⚡
Mapping::IS_DYNAMICdefaults tofalse(storage/types/mod.rs:182) —Mappingis keccak-addressed but does not implementStorable(onlyStorableType), so the derive splits mapping fields out before callingstore/load/delete. Harmless today, but if a future change adds aStorableimpl toMapping, the propagation logic ingen_store_implwould need updating.
| let mut data_slot: Option<U256> = None; | ||
|
|
||
| // (T5+) Cleanup stale tail if necessary. | ||
| if !ctx.skip_tail_cleanup() && StorageCtx.spec().is_t5() { |
There was a problem hiding this comment.
🛡️ [DEFENSE-IN-DEPTH] store_bytes_like cleanup misses pre-T5 ghost chunks
This branch keys on whether the current base slot decodes as a long string (is_long_string(prev) below). If a string was long pre-T5, then overwritten short pre-T5 (so the base reads short but the keccak data slots still hold the original long-string bytes), then post-T5 overwritten with another long value of fewer chunks than the original, the cleanup sees is_long_string(prev) = false and skips clearing. The keccak slots beyond the new length retain ghost bytes from the original pre-T5 write.
Reads remain correct (length-bounded), so this is not a consensus or correctness issue. But the implicit "after T5, slots beyond the recorded length are zero" invariant does not hold across the activation boundary. The same caveat applies to Vec::store's use of prev_len from len_slot.
Recommended Fix:
Either (a) document explicitly in the doc-comment above that the guarantee is "operations-from-T5-onward" rather than absolute, so future hardforks that consume keccak-derived data slots don't read ghost bytes; or (b) ship a one-shot T5-activation migration to scrub legacy tails. Option (a) is sufficient unless a future fork plans to reuse those slots.
|
|
||
| // (T5+) Cleanup stale tail, if necessary. | ||
| if !ctx.skip_tail_cleanup() && StorageCtx.spec().is_t5() { | ||
| let (prev_len, new_len) = (load_checked_len(storage, len_slot)?, self.len()); |
There was a problem hiding this comment.
💡 [SUGGESTION] New T5 failure mode if len_slot was repurposed pre-T5
On T5, Vec::store now reads prev_len via load_checked_len, which errors if the raw value > u32::MAX. If a slot at len_slot was ever populated with a value > u32::MAX by something other than a Vec (e.g., schema reassignment / cross-type slot reuse), a Vec write to that slot will start failing on T5 where it previously succeeded pre-T5. No production code does this today, but worth checking before T5 activation that no precompile upgrade re-purposes a Slot<U256> (or similar) as a Slot<Vec<…>>.
Recommended Fix: Audit precompile storage layouts for cross-type slot reuse before T5 activation. No code change required if none exists.
Collects bug fixes and infrastructure changes gated behind T5: 1. Fix fixed-size array packing in precompile storage codegen (#3811) 2. Clean up stale tail slots in dynamic storage types (#3840) 3. Deploy ERC-2470 and NanoUniversalDeployer at T5 boundary (#3742) Standalone feature TIPs (1026, 1030, 1033, 1035, 1047, 1056) are excluded — they already have their own specs. Amp-Thread-ID: https://ampcode.com/threads/T-019dfe69-9662-77ff-9ff0-390655ec07ff
Collects bug fixes gated behind T5: 1. Fix fixed-size array packing in precompile storage codegen (#3811) 2. Clean up stale tail slots in dynamic storage types (#3840) Standalone feature TIPs (1026, 1030, 1033, 1035, 1047, 1056) are excluded — they already have their own specs. Amp-Thread-ID: https://ampcode.com/threads/T-019dfe69-9662-77ff-9ff0-390655ec07ff
3 participants
Motivation
Overwriting a dynamic storable (
Vec<T>,String,Bytes) with a shorter value left stale tail slots populated, so subsequent reads observed garbage past the new length.Solution
Ensure that "shrinking" writes on dyn types clear their stale tails.
Additionally, introduces a new sentinel
LayoutCtx::INITfor hot paths that know the destination is virgin (Vec::push), letting them skip the extra SLOAD + cleanup.