fix(types)!: reject malformed SignedOrder JSON at deserialize time by prestwich · Pull Request #237 · init4tech/signet-sdk

prestwich · 2026-05-21T10:45:30Z

Summary

Fix ENG-2288: SignedOrder::order_hash() panicked on a SignedOrder deserialized from untrusted JSON whose signature was not exactly 65 bytes — an asymmetric DoS vector against any consumer of the tx-cache order feed.
Promote structural invariants (signature length, non-empty permitted, non-empty outputs) to type-system guarantees enforced at deserialize time, so order_hash's Signature::from_raw(...).unwrap() becomes sound by construction.
Reusable signet_zenith::serde_helpers exposes deserialize_signature_bytes and deserialize_non_empty_vec for other sol-shaped types that need the same treatment at JSON boundaries.
Rename SignedOrder::new → new_unchecked (documenting the invariants the caller must uphold) and SignedOrder::validate → validate_at, plus a new is_valid_at, drawing a clear line between structural validity (enforced at construction) and time/state validity (checked at call sites).
Proptests in signet-test-utils/tests/deserialization_fuzz.rs prove the SignedOrder deserialize path is total — every input either deserializes into a value satisfying all three invariants, or errors via serde; never panics.
// SAFETY: annotations on two audit findings (agg/fill.rs:209, agg/order.rs) whose unwraps/casts are sound under caller-side or protocol invariants.

Breaking changes

SignedOrder::new(...) → SignedOrder::new_unchecked(...)
SignedOrder::validate(...) → SignedOrder::validate_at(...)
<SignedOrder as Deserialize> now rejects inputs that previously deserialized successfully (i.e. the bug).

Minor-version bump recommended on next release.

Reporter

This bug was reported externally by Cinder Circuit / @achimala under the Signet bug bounty.

Test plan

cargo t -p signet-types -p signet-zenith -p signet-orders -p signet-test-utils — all pass
cargo clippy --workspace --all-targets --all-features -- -D warnings
cargo clippy --workspace --all-targets --no-default-features -- -D warnings
RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps
cargo +nightly fmt -- --check
Reproduction test for the bounty bug flipped from #[should_panic] to asserting a serde Err
Proptest coverage: helper boundaries, SignedOrder totality, round-trip stability, garbage-string safety
Reviewer to confirm tx-cache stream consumers surface deserialize errors gracefully rather than terminating the stream when a malformed order arrives

🤖 Generated with Claude Code

Fixes ENG-2288

`SignedOrder::order_hash()` previously panicked on a `SignedOrder` deserialized from JSON when the `signature` field was not exactly 65 bytes, via `Signature::from_raw(...).unwrap()`. The same type is used for both ABI-decoded chain events (where the 65-byte invariant is enforced by the contract) and serde-decoded JSON (where any byte string is accepted), so the unwrap was unsound at the off-chain trust boundary. This change: - Adds `signet_zenith::serde_helpers` with `deserialize_signature_bytes` and `deserialize_non_empty_vec` for use as `#[serde(deserialize_with)]` on sol-shaped types received at JSON boundaries. - Replaces `SignedOrder`'s derived `Deserialize` with a custom impl backed by private repr types that enforce a 65-byte signature, a non-empty `permitted` list, and a non-empty `outputs` list. Malformed inputs now surface as `serde_json::Error`. - Renames `SignedOrder::new` to `new_unchecked` (the structural invariants are now documented on the constructor). - Renames `SignedOrder::validate` to `validate_at` and adds `is_valid_at`, clarifying that this checks time/state validity only. - Documents the `order_hash` `.unwrap()` as sound under the type-level invariant, with a `// SAFETY:` comment. - Adds proptests in `signet-test-utils` proving the deserialize path never panics on arbitrary input. - Adds `// SAFETY:` annotations to two related audit findings in `agg/fill.rs` and `agg/order.rs` whose unwraps/casts are sound under caller-side or protocol invariants. Refs ENG-2288. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Breaking changes in this release: - `SignedOrder::new` renamed to `new_unchecked` - `SignedOrder::validate` renamed to `validate_at` - `<SignedOrder as Deserialize>` now rejects malformed signatures, empty `permitted`, and empty `outputs` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Fraser999

One blocker on the issue in checked_remove_aggregate.

…ing recipient check_aggregate accepted a zero-amount output to a recipient absent from self.fills (filled() collapses missing-recipient and zero-balance into U256::ZERO, so 0 < 0 is false), then checked_remove_aggregate panicked unwrapping the inner get_mut. Guard the inner lookup with `if let Some` so the absent case becomes a no-op; check_aggregate semantics are unchanged. Refactor the two check_filled methods into one private helper that takes a pre-resolved balance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The test asserts that an empty vec is rejected; the previous name read as the opposite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Fraser999 · 2026-05-28T11:31:20Z

+    if v.is_empty() {
+        return Err(D::Error::invalid_value(Unexpected::Seq, &"at least one element"));
+    }


[Claude Code]

Length alone isn't a sufficient invariant for compute_order_hash_pre_image's Signature::from_raw(...).unwrap(). from_raw also rejects 65-byte payloads whose v byte falls outside {0, 1, 27, 28, 35..} (alloy normalize_v), so the original ENG-2288 panic is still reachable via a 65-byte signature with v = 5 (or any other invalid parity):

Suggesting to drop the manual length check and delegate everything to Signature::from_raw, which checks both length and parity in one place:

Suggested change

if v.is_empty() {

return Err(D::Error::invalid_value(Unexpected::Seq, &"at least one element"));

}

Signature::from_raw(&bytes).map_err(D::Error::custom)?;

This keeps the helper's promise (post-deserialize bytes are safe to feed straight into Signature::from_raw) as a single source of truth against the downstream consumer, instead of two checks that can drift. The "expected exactly 65 bytes" substring is still present in the length-error path, so the existing assertion tests pass without changes.

The current proptest doesn't catch this because sig_bytes in 0..130 only produces a 65-byte input ~0.77% of the time - under default 256 cases, expected hits with len==65 ∧ invalid v is ≈0.24, so a green run is mostly luck. Worth adding a dedicated 65-byte-with-arbitrary-v strategy and a hard-coded v=5 regression test once the helper is tightened.

Evalir

Three points from review — flagging inline.

Evalir · 2026-05-28T12:39:17Z

@@ -61,25 +61,6 @@ impl AggregateFills {
        self.fills.get(output_asset).and_then(|m| m.get(&recipient)).copied().unwrap_or_default()
    }


Undisclosed breaking change (review issue 1): the sibling pub fn check_filled on AggregateFills is removed in this PR (replaced by a private module-level helper). That's a public-API removal in signet-types, but the PR description's Breaking changes section lists only the SignedOrder renames. Worth adding here so the 0.19 → 0.20 bump's changelog is complete and downstream consumers aren't surprised. (The analogous removal on CombinedContext::check_filled is fine — that one was private.)

Evalir · 2026-05-28T12:39:17Z

        buf.extend_from_slice(keccak256(self.outputs.abi_encode()).as_slice());

-        // Normalize the signature.
+        // SAFETY: `self.permit.signature` is exactly 65 bytes by the


Style nit (review issue 2): // SAFETY: is the established Rust convention for justifying unsafe blocks. The reasoning here is load-bearing — a non-unsafe invariant explanation tied to new_unchecked's contract — but using the SAFETY keyword on safe code blurs the convention and can confuse readers grepping for actual unsafe usage. Consider // INVARIANT: or just a plain comment lead-in. The content of the comment is excellent; only the keyword is the nit. Same applies to the // SAFETY: notes in agg/fill.rs and agg/order.rs.

Evalir · 2026-05-28T12:39:17Z

        self.check_aggregate(aggregate)?;

-        for (output_asset, recipients) in aggregate.outputs.iter() {
+        // SAFETY: `check_aggregate` above guarantees that, for every


Style nit (review issue 2, cont.): same // SAFETY: → // INVARIANT: (or plain comment) suggestion as in signing/order.rs. The reasoning itself is correct and well-written — the 0 < 0 == false argument for the absent-inner-entry case is the right justification.

Evalir · 2026-05-28T12:39:17Z

                        token: *token,
                        amount: U256::from(*amount),
                        recipient: *recipient,
+                        // SAFETY: signet chain IDs are protocol-defined to


Style nit (review issue 2, cont.): // SAFETY: → // INVARIANT: or plain comment, same reasoning as the other two sites.

Separately (optional, non-blocking): ru_chain_id as u32 will silently truncate if a future protocol change ever lifts the uint32 constraint on Output.chainId. u32::try_from(ru_chain_id).expect("signet chain IDs fit in u32 — see protocol invariant") would panic loudly at the boundary instead of producing a wrong chain ID. The comment already flags this as the thing to revisit, so it's a matter of taste whether to encode the check in code too.

Evalir · 2026-05-28T12:39:17Z

+/// `initiatePermit2` contract entrypoint: a 65-byte signature, a non-empty
+/// `permitted` list, and a non-empty `outputs` list.
+#[derive(Deserialize)]
+struct SignedOrderRepr {


Maintainer note worth adding (review issue 3): SignedOrder now has a hand-rolled Deserialize (via this SignedOrderRepr) but a derived Serialize. That asymmetry is fine today and the roundtrip tests catch divergence — but if a sol-emitted field is ever renamed/added inside Permit2Batch, TokenPermissions, or Output, the derived Serialize will silently follow the change while SignedOrderRepr will not, and the only signal will be a roundtrip-test failure.

A short comment on SignedOrderRepr pointing future maintainers at this coupling — "keep field shape in sync with the derived Serialize on SignedOrder; roundtrip tests will catch drift" — would save the next person a debugging session.

no field will ever be added, as the contracts are finalized

prestwich · 2026-05-30T07:25:25Z

these comments feel super pedantic and not really worth addressing

prestwich requested a review from a team as a code owner May 21, 2026 10:45

prestwich requested review from Evalir and Fraser999 May 21, 2026 10:49

Fraser999 requested changes May 21, 2026

View reviewed changes

Comment thread crates/zenith/src/serde_helpers.rs Outdated

Comment thread crates/types/src/agg/fill.rs Outdated

prestwich and others added 2 commits May 28, 2026 04:02

test(zenith): rename non_empty_vec_rejected to empty_vec_rejected

3e95876

The test asserts that an empty vec is rejected; the previous name read as the opposite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Fraser999 reviewed May 28, 2026

View reviewed changes

Evalir reviewed May 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(types)!: reject malformed SignedOrder JSON at deserialize time#237

fix(types)!: reject malformed SignedOrder JSON at deserialize time#237
prestwich wants to merge 4 commits into
mainfrom
prestwich/eng-2288

prestwich commented May 21, 2026 •

edited

Loading

Uh oh!

Fraser999 left a comment

Uh oh!

Uh oh!

Uh oh!

Fraser999 May 28, 2026

Uh oh!

Evalir left a comment

Uh oh!

Evalir May 28, 2026

Uh oh!

Evalir May 28, 2026

Uh oh!

Evalir May 28, 2026

Uh oh!

Evalir May 28, 2026

Uh oh!

Evalir May 28, 2026

Uh oh!

prestwich May 30, 2026

Uh oh!

prestwich commented May 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -61,25 +61,6 @@ impl AggregateFills {
		self.fills.get(output_asset).and_then(\|m\| m.get(&recipient)).copied().unwrap_or_default()
		}

Conversation

prestwich commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking changes

Reporter

Test plan

Uh oh!

Fraser999 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fraser999 May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Evalir May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Evalir May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Evalir May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Evalir May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Evalir May 28, 2026

Choose a reason for hiding this comment

Uh oh!

prestwich May 30, 2026

Choose a reason for hiding this comment

Uh oh!

prestwich commented May 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prestwich commented May 21, 2026 •

edited

Loading