Skip to content

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

Open
prestwich wants to merge 2 commits into
mainfrom
prestwich/eng-2288
Open

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

Conversation

@prestwich
Copy link
Copy Markdown
Member

@prestwich prestwich commented May 21, 2026

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::newnew_unchecked (documenting the invariants the caller must uphold) and SignedOrder::validatevalidate_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>
@prestwich prestwich requested a review from a team as a code owner May 21, 2026 10:45
@prestwich prestwich requested review from Evalir and Fraser999 May 21, 2026 10:49
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>
Copy link
Copy Markdown
Contributor

@Fraser999 Fraser999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One blocker on the issue in checked_remove_aggregate.

}

#[test]
fn non_empty_vec_rejected() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn non_empty_vec_rejected() {
fn empty_vec_rejected() {

Comment on lines +215 to +221
// SAFETY: the `check_aggregate` call above proves, for every
// `(output_asset, recipient)` pair in `aggregate`, that the entry
// exists in `self.fills` and that `filled >= amount`. We hold
// `&mut self` for the duration, so neither the map nor the
// recipient balances can change between the check and the
// mutation. The `get_mut`/`unwrap` and `checked_sub`/`unwrap`
// below are therefore infallible by construction.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude has spotted that this isn't really true. The following snippet probably explains the edge case best:

let mut context = AggregateFills::default();
let asset = Address::with_last_byte(1);
let present_recipient = Address::with_last_byte(99);
let missing_recipient = Address::with_last_byte(2);

// The outer `(chain, asset)` key exists in `self.fills`, but only `present_recipient` is in the inner map.
context.add_raw_fill(1, asset, present_recipient, U256::from(100));

// Zero-amount output to a recipient that is absent from `self.fills[(1, asset)]`.
// `check_aggregate` returns `Ok` here.
let mut aggregate = AggregateOrders::new();
aggregate.ingest_raw_output(1, asset, missing_recipient, U256::ZERO);

// Sanity check: the prior check the SAFETY comment leans on is currently a no-op for this input.
context.check_aggregate(&aggregate).unwrap();

// The unwrap inside `checked_remove_aggregate` panics here.
let _ = context.checked_remove_aggregate(&aggregate);

We probably want to tighten check_aggregate to fail for this case (and add a regression test)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants