Skip to content

fix: pin SS58 prefix to 189 and harden Rust error handling (L5, L6)#581

Open
n13 wants to merge 3 commits into
mainfrom
fix/l5-l6-rust-hardening
Open

fix: pin SS58 prefix to 189 and harden Rust error handling (L5, L6)#581
n13 wants to merge 3 commits into
mainfrom
fix/l5-l6-rust-hardening

Conversation

@n13

@n13 n13 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses findings L5 and L6 of the 2026-07-22 mobile wallet security audit.

L5 — Foreign SS58 network prefixes accepted as valid recipients

  • ss58_to_account_id (quantus_sdk/rust/src/api/crypto.rs) now decodes with AccountId32::from_ss58check_with_version and rejects any prefix other than the Quantus prefix 189 (verified against the chain runtime's Ss58AddressFormat::custom(189) and AppConstants.ss58prefix). Previously any registered prefix (Polkadot 0, Kusama 2, generic 42, ...) validated and decoded to an uncontrolled AccountId32, burning funds.
  • Dart-side isValidSS58Address and all other ss58ToAccountId call sites keep working unchanged: the FRB Dart signature is still Uint8List ss58ToAccountId(...), it now throws on invalid/foreign input instead of aborting on a Rust panic. Foreign prefixes are now rejected.

L6 — Rust API panic style on user-controlled input

  • ss58_to_account_id: unwrapResult<Vec<u8>, String> (matches the existing Result<T, String> pattern used throughout wormhole.rs / first_hash_to_address).
  • generate_keypair: expectResult<Keypair, HDLatticeError> (same error type already used by generate_derived_keypair / derive_wormhole).
  • wormhole_compute_output_amount: checked arithmetic (checked_sub for the fee_bps > 10000 underflow, checked_mul, u32::try_from) → Result<u32, String>.
  • decode_leaf_amount: silent as u32 truncation → checked u32::try_from with an error. Kept the u32 return type (consistent with ProofInput amount fields) so the Dart signature is unchanged.
  • Removed the dead, panicking derive_hd_path FRB export — verified zero Dart callers outside the generated bindings (grepped quantus_sdk/lib, mobile-app/lib, miner-app/lib, cold-wallet-app/lib). Its only dependency, nam-tiny-hderive, was removed from Cargo.toml.

Bindings regeneration

FRB bindings were regenerated with flutter_rust_bridge_codegen generate (v2.12.0, matching the pinned flutter_rust_bridge = "=2.12.0"). The public Dart API is source-compatible: all changed functions keep their Dart return types and now throw instead of panicking; all existing call sites already wrap these calls in try/catch or propagate exceptions.

Note: the local Dart formatter (SDK 3.12.0) re-wrapped parts of the regenerated files; purely mechanical formatting churn in frb_generated.*. Formatting-only changes to the untouched api/multisig.dart and api/ur.dart were reverted to keep the diff minimal.

Test fixes

  • generate_keys_test.dart: the wormhole test compared against a generic Substrate (prefix 42) address; replaced with the prefix-189 encoding of the same account bytes, and added a test asserting ss58ToAccountId rejects the foreign-prefix and garbage inputs.
  • recovery_proxy_encoding_test.dart: setUpAll now calls setDefaultSs58Prefix(prefix: 189) (it only ran RustLib.init(), so toAccountId encoded with the sp-core default prefix 42, which is now rejected on decode).

Verification

  • cargo check — clean
  • cargo clippy — clean (one pre-existing needless_range_loop warning in untouched generate_proof code)
  • cargo test --lib — 13/13 passed
  • dart analyze (quantus_sdk) — no issues
  • flutter test (quantus_sdk, full suite, incl. native tests against a release build of the Rust lib) — 134/134 passed

Scope notes

  • rust-transaction-parser was inspected per the audit scope and left alone — no SS58 decoding there.
  • Other expects on internally-generated key material (to_ml_dsa, generate_keypair_from_seed, sign_message) were left as-is; the audit flags only the user-controlled entry points fixed here.

@n13

n13 commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

🔍 Review — L5, L6 (SS58 prefix pinning & Rust error hardening)

Verdict: 🟡 Approve with non-blocking comments

The core L5/L6 remediation is correct and cleanly executed: ss58_to_account_id now decodes with from_ss58check_with_version and rejects any prefix ≠ 189, the panic→Result conversions propagate through FRB without changing the public Dart signatures, and the regenerated bindings are internally consistent. The only substantive observation is a second, un-pinned user-controlled SS58 decode site (predict_multisig_address) that this PR leaves alone — low-risk, but inconsistent with the new policy.

What it does

  • Pins ss58_to_account_id (quantus_sdk/rust/src/api/crypto.rs:60) to the Quantus prefix 189 via AccountId32::from_ss58check_with_version; foreign/garbage input now returns Err instead of decoding an uncontrolled AccountId32 (L5).
  • Converts user-facing panics to Result (L6): ss58_to_account_id (unwrapResult<Vec<u8>,String>), generate_keypair (expectResult<Keypair,HDLatticeError>), wormhole_compute_output_amount (checked sub/mul + u32::try_fromResult<u32,String>), decode_leaf_amount (as u32u32::try_from).
  • Removes the dead, panicking derive_hd_path FRB export and its nam-tiny-hderive dependency (+ transitive base58).
  • Regenerates FRB bindings; fixes two stale Dart tests and adds a foreign-prefix/garbage rejection test.

Strengths

  • 189 is the confirmed Quantus prefix — consistent across quantus_sdk/lib/src/constants/app_constants.dart:62, quantus_sdk/rust/src/api/multisig.rs:6, quantus_sdk/lib/generated/planck/pallets/system.dart:792, rust-transaction-parser/src/lib.rs:347, and quantus_sdk/lib/quantus_sdk.dart:106 (setDefaultSs58Prefix(189) at init).
  • Test change is a legitimate correction, not masking. I independently base58-decoded both test addresses: 5H8AGzwKPtKM… (prefix 42) and qzpWh4AEtsgC… (prefix 189) resolve to the identical 32-byte pubkey dfcfd6e5…404d0e. The old test only passed because foreign prefixes decoded to the same bytes; the swap to the prefix-189 encoding of the same account keeps the equality assertion valid while adapting to the intended new reject-on-42 behavior (quantus_sdk/test/generate_keys_test.dart:100-108). The setDefaultSs58Prefix(prefix: 189) in recovery_proxy_encoding_test.dart:31 is the analogous fix (encode side must now match the pinned decode side).
  • No unintended FRB signature drift. The funcId renumbering (13–34 shifted −1 after derive_hd_path removal) is consistent between frb_generated.dart and frb_generated.rs; the only codec changes are the three decodeErrorData additions matching the three functions that became Result (HDLatticeError for generate_keypair, sse_decode_String for ss58_to_account_id and wormhole_compute_output_amount). Public Dart signatures stay Uint8List ss58ToAccountId, Keypair generateKeypair, int wormholeComputeOutputAmount, int decodeLeafAmount — they now throw instead of aborting on a Rust panic.
  • Clean error propagation. HDLatticeError is a pre-existing FRB-known type (crypto.rs:5 re-export, already used by generate_derived_keypair/derive_wormhole), so mnemonic_to_seed(…)? and transform_result_sse::<_, HDLatticeError> compile and propagate without a new binding type.
  • derive_hd_path removal is truly free. Grep confirms the only references outside generated bindings were the definition itself; no callers in quantus_sdk/lib, mobile-app/lib, miner-app/lib, cold-wallet-app/lib. Cargo.toml/Cargo.lock correctly drop nam-tiny-hderive (and transitive base58, signature).
  • Correct integration, fail-closed. isValidSS58Address (substrate_service.dart:418) already wraps the call in try/catch, so foreign/garbage now returns false — the send/recipient/hardware screens now reject foreign-network addresses (the intended fix). The direct, un-caught call sites (balances_service, recovery_service, reversible_transfers_service) now throw rather than silently build a burn transaction — an improvement, and behaviorally equivalent to the old panic for those paths.

Findings

  1. [non-blocking] Second user-controlled SS58 decode site left un-pinned — predict_multisig_address (quantus_sdk/rust/src/api/multisig.rs:39). It decodes user-supplied signer addresses with plain AccountId32::from_ss58check(s) (no prefix check), so it still accepts Polkadot/Kusama/42 addresses — the exact class L5 targets. Risk is genuinely low here because (a) it already returns Result (no panic, so L6 is satisfied) and (b) a foreign-prefix address decodes to the same 32-byte account, so the predicted multisig address is unchanged. But it is now inconsistent with ss58_to_account_id's policy; for defense-in-depth/consistency it should also reject prefix ≠ 189 (or reuse a shared helper). Worth a follow-up rather than a blocker.
  2. [non-blocking] generate_keypair_from_seed (quantus_sdk/rust/src/api/crypto.rs:110) is still an FRB-exported function that panics via .expect("Seed must be 32 bytes"). The PR scope-notes it as "internal," and grep confirms no non-test app callers of generateKeypairFromSeed — so it is not reachable with attacker input in practice — but it is on the public FRB surface, unlike the other entry points that were hardened. Acceptable to defer.
  3. [nit] Thin negative-test assertions and no coverage of the new arithmetic error paths. The rejection test uses throwsA(anything) (generate_keys_test.dart:113-114) — it proves a throw but not which error, and doesn't assert prefix-vs-format distinction. There are also no tests for wormhole_compute_output_amount (fee_bps > 10000, checked_mul overflow) or decode_leaf_amount u32::try_from overflow. Secondary hardening, so non-blocking.
  4. [nit] wormhole_compute_output_amount has no Dart callers outside the generated wrapper (quantus_sdk/lib/src/rust/api/wormhole.dart:20). The hardening is correct, but the function is currently unused by app code — flag only so the maintainer is aware it's exercised solely via the FRB surface.

Verification

Adequate for the primary finding. The added Dart test directly asserts that a foreign-prefix (42) address and garbage both throw, which is the crux of L5 at the fixed entry point, and I independently confirmed the prefix-189 replacement address is byte-equivalent to the original. PR-body verification claims (cargo check/clippy clean, cargo test --lib 13/13, flutter test 134/134) are consistent with the diff and could not be independently re-run under the read-only constraint. Gaps: no unit test for the wormhole checked-arithmetic branches or the decode_leaf_amount overflow branch, and the multisig decode path (Finding 1) has a garbage-rejection test but no foreign-prefix-rejection test because it isn't pinned.


🤖 AI-assisted review generated with Claude Code

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

LGTM

Resolve Cargo.toml/Cargo.lock conflicts by taking main's updated dependency
set (qp-wormhole-* 3.1.0, sp-core 43, etc.) and dropping the now-unused
nam-tiny-hderive dep that this branch removed. Cargo.lock regenerated;
`cargo check` passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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.

1 participant