fix: veridise audit feb 2026 issues - #67
Merged
Merged
Conversation
Changing so that CI runs on PRs targetting the veridise-audit-feb-2026 branch.
Fixes Veridise-905. Veridise identified a footgun where FlaggedStorage has a lot of implicit conversions to and from U256 and FixedBytes<32>, making it easy to make errors and silently drop a privacy flag. Claude reported that there is no bug related to this in our current codebase, but that it does make easy to do accidentally. Having impl From<FlaggedStorage> for U256 makes it trivial to silently drop privacy in a refactor. For example, any code that calls collect_value() (line 106-110) and then converts back would lose all privacy flags. ## Fix Opted to remove both directions: 1. Remove From<FlaggedStorage> for U256 and From<FlaggedStorage> for FixedBytes<32>. Replace with an explicit .value accessor pattern. This makes the lossy conversion visible at every call site. It's a breaking change but a small one — callers already have access to the .value field. 2. Remove the primitive → FlaggedStorage impls (the generic From<T> at line 22-29 and the Into<FlaggedStorage> for FixedBytes<32> at line 37-42). Force callers to use FlaggedStorage::public(v) or FlaggedStorage::private(v) — making the privacy choice explicit at construction. ## Downstream Impact The blast radius is significant — ~50+ call sites across 7 repos, though most are test code and easy to fix. - Step 1 — seismic-alloy-core: Remove all 5 conversion impls - Step 2 — Each downstream repo: Replace with explicit .value / FlaggedStorage::public() / FlaggedStorage::new(v, is_private) ### Seismic-trie fix The trickiest part is seismic-trie's T: Into<FlaggedStorage> bounds on the storage_root* functions — those will need to change to accept FlaggedStorage directly. The SEISMIC WARNING comments on lines 68, 77, 92 even acknowledge the footgun: > "Ensure that the storage values are flagged correctly when calling" That warning exists precisely because the T: Into<FlaggedStorage> bound allows passing a bare U256 Upstream uses concrete (B256, U256): ``` // upstream alloy-trie pub fn storage_root(storage: impl IntoIterator<Item = (B256, U256)>) -> B256 ``` Seismic changed it to: ``` // seismic-trie pub fn storage_root<T: Into<FlaggedStorage>>(storage: impl IntoIterator<Item = (B256, T)>) -> B256 ``` The Into<FlaggedStorage> was a mechanical replacement for U256 that preserved the generic flexibility, but it's the wrong abstraction here — it lets callers pass a bare U256 and silently get is_private: false. The warning comments are a band-aid acknowledging this. The fix for seismic-trie should just be making it concrete like upstream was: ``` pub fn storage_root(storage: impl IntoIterator<Item = (B256, FlaggedStorage)>) -> B256 ``` This forces callers to explicitly construct a FlaggedStorage with the right privacy flag before calling, which is exactly what removing the From<T> impls achieves too. Both changes reinforce each other.
Fixes veridise-899. The sint_impls! macro for shielded types (sint/suint) panicked for any width < 256 bits: - Encode: word[..].copy_from_slice(N/8 bytes) tried to copy into all 32 bytes → fixed to word[32-N/8..] - Decode: token.0[..].try_into().unwrap() tried to convert 32 bytes into [u8; N/8] → fixed to read from token.0[32-N/8..] - Sign extension: Signed tokenization used Word::ZERO instead of 0xff-fill for negatives Fixed all of these issues. Also deleted the unnecessary sint_impls2!/suint_impls2! forwarding macros (inlined into supported_sint!), and added a comment explaining why seismic uses ruint for all widths instead of splitting into primitive vs big like upstream. ## Testing Added a regression test covering round-trip encode/decode across representative widths (8, 16, 24, 32, 40, 48, 56, 64, 128, 200, 248, 256) for signed, unsigned, and negative values.
…66) Fixes veridise-904. PACKED_ENCODED_SIZE was wrongly hardcoded to 32 for all shielded types. My guess is this was done by mistake due to comparing to our compiler's behavior (see section below). ## Impact (Claude analysis) PACKED_ENCODED_SIZE is used in two places: 1. stv_abi_packed_encoded_size() (lib.rs:108-109) — default impl returns T::PACKED_ENCODED_SIZE.unwrap(). This is used for buffer preallocation in abi_encode_packed() and for computing packed sizes of tuples/arrays. However, none of the seismic types override this method, so they'll return 32, while their stv_abi_encode_packed_to() writes fewer bytes. This means the preallocated buffer will be slightly too large (wastes memory, but doesn't corrupt data). 2. dyn-abi (DynValue::abi_packed_encoded_size) (value.rs:876-898) — this one is already correct! It hardcodes the right sizes: Sbool → 1, Saddress → 20, Sint → size/8, Suint → size/8. It doesn't read the PACKED_ENCODED_SIZE constant at all. So the mismatch causes: - Slight over-allocation when Vec::with_capacity uses the packed size — harmless, just wastes a few bytes - No data corruption — the actual encoding is correct, just the size hint is wrong - Potential issues in tuple packed size computation (data_type.rs:732-734) — if a tuple contains shielded types, the computed packed size will be too large, again causing over-allocation but not corruption ## Behavior here vs solidity compiler slot packing Note that this is different from our [solidity compiler's packing behavior](https://github.com/SeismicSystems/seismic/blob/main/docs/language-and-vm.md#2-storage-behavior), where we cannot pack public and private state variables into a single slot since FlaggedStorage applies the `is_shielded` bool to the entire slot. The packing behavior here is purely for people in rust doing `abi.packedEncoding()`. We could make it follow the compiler's behavior, but I can't think of a good reason why to.
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
2 participants
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.
Make sure to rebase instead of merging this PR, so we keep the history of the independent PRs.