feat!: siegel key manager for Safe Smart Account 🧧#355
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ba38efb96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
NnnOooPppEee
left a comment
There was a problem hiding this comment.
Security review of the Siegel integration. Two inline notes below. One additional pre-existing observation that doesn't touch this PR's diff:
EoaSigner::new at signer.rs L260-L267 — pre-existing in this file (not from this PR): hex::decode(private_key) returns a plain Vec<u8> with the raw 32 key bytes, and Vec::drop doesn't zero memory — so after new() returns, those bytes sit in the allocator's free list until something reuses the slot. Same pattern as TOB-Parity-022 (p.23). One-line fix: Zeroizing::new(hex::decode(private_key)?). Worth folding in here since EoaSigner will need the same treatment when it migrates to SmartAccountKeyManager anyway.
Full write-up and the equivalent upstream Siegel hardening PR are tracked separately.
| zeroize = "1.8" | ||
| hex-literal = "1.1.0" # Provides the `hex!` macro for compile-time hex decoding | ||
| http = "1.4.0" | ||
| siegel-uniffi = "0.1.0" |
There was a problem hiding this comment.
siegel-uniffi = "0.1.0" resolves under Cargo's caret semver to >=0.1.0, <0.2.0 rather than an exact pin, and it's published from the personal paolodamico crates.io account. cargo update or Dependabot can pull a future 0.1.x without code review. For a dep sitting in the EOA signing path that's a wider supply-chain surface than ideal.
Smallest fix: siegel-uniffi = "=0.1.0". Longer term a Worldcoin-owned mirror or namespace transfer would be cleaner.
|
|
||
| // Read the key once to validate it and derive the EOA address. | ||
| // The session is consumed and zeroized inside `read_once`. | ||
| let siegel = key_manager.get_eoa_private_key(); |
There was a problem hiding this comment.
Worth adding a length check at the FFI boundary here. hex::decode + LocalSigner::from_slice silently accepts 24-31 byte inputs and zero-pads them (k256's SecretKey::from_slice MIN_SIZE branch). A foreign-side bug that allocates a smaller SiegelSession would sign as a different EOA than the user expects — same wallet from their UI, different on-chain identity.
Smallest fix: if siegel.len() != 64 { Err(...) } + hex::decode_to_slice into a stack-pinned Zeroizing<[u8; 32]>. Same fix applies in sign_hash_sync.
I'll send a parallel PR upstream in paolodamico/siegel so siegel_fill can also enforce expected length, so future callers don't have to remember.
Introduces Siegel to securely pass the secret bytes of the EOA private key to Rust for one-off signature operations. This ensures the private key exists in application memory solely for the duration of the signature process and then zeroized (and while the key is in memory there's protections against under/overflows).
Outstanding
Other places still pass secrets to Rust via the UniFFI boundary (e.g. backup manager) and must be updated.
Note
High Risk
High risk because it changes how Safe EOA private keys are provided and used for all signing operations (new UniFFI foreign trait + per-call key retrieval/zeroization), and it is a breaking API change for
SafeSmartAccount::newconstruction.Overview
Safe Smart Account key handling is refactored to stop passing raw private keys into Rust.
SafeSmartAccount::newis now a breaking UniFFI constructor that takes a foreignSmartAccountKeyManagerand pulls the EOA key via per-callSiegelSessionreads (zeroized immediately), while caching the derived EOA address at construction.Adds
siegel-uniffiplumbing (uniffi_reexport_scaffolding!), introducesSmartAccountKeyManagerand testInMemoryKeyManager, mapsSessionErrorinto a newSafeSmartAccountError::SiegelSession, and updates all affected tests/docs to usefrom_private_key_hex(test-only) instead of the old constructor.Reviewed by Cursor Bugbot for commit 2a982e8. Bugbot is set up for automated code reviews on this repo. Configure here.