fix(l2): add EIP-2 low-S signature validation to ZisK's recover_signer#6469
fix(l2): add EIP-2 low-S signature validation to ZisK's recover_signer#6469avilagaston9 wants to merge 3 commits intomainfrom
Conversation
ZisK's recover_signer was passing signatures directly to the FFI function without checking that S is in the lower half of the curve order. This means high-S (malleable) transaction signatures would be accepted by ZisK but rejected by all other backends (SP1, RISC0, OpenVM), which enforce the check via k256_recover_signer in shared.rs. The ecrecover precompile (secp256k1_ecrecover) is intentionally left without this check, matching Ethereum spec behavior where the precompile accepts any valid signature regardless of S value. Also updates the stale ZiskCrypto docstring that still referenced the old k256/substrate-bn patched crate approach.
🤖 Kimi Code ReviewReview for PR #6469 The changes correctly implement EIP-2 signature malleability protection by rejecting high-s values ( Code correctness:
Minor suggestions:
Security: The check prevents signature malleability attacks where Verdict: Correct and safe. No blocking issues. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewFindings No blocking findings. The change in zisk.rs:95 looks correct: it brings Residual Risk
I could not run the relevant test locally because the sandbox blocks Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview: Add EIP-2 low-S signature validation to ZisK's
|
avilagaston9
changed the title
Lines of code reportTotal lines added: Detailed view |
🤖 Kimi Code ReviewOverall Assessment: The PR correctly implements EIP-2 signature malleability protection and updates documentation. The implementation is sound with correct constant values and proper bounds handling. Specific Feedback:
Minor Suggestion: // EIP-2: reject high-s signatures to prevent signature malleability
if sig[32..64] > SECP256K1_N_HALF[..] {Security: This change is consensus-critical. The implementation correctly rejects Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR adds the EIP-2 low-S constraint check ( Confidence Score: 5/5Safe to merge — the change is a minimal, correct security fix with no P0/P1 findings. The constant value and comparison logic are identical to the already-validated check in No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/guest-program/src/crypto/zisk.rs | Adds the EIP-2 low-S check (s > SECP256K1_N_HALF → InvalidSignature) to recover_signer before the FFI call, and updates the stale doc comment — logic is correct and consistent with the other backends. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["recover_signer(sig, msg)"] --> B{"s = sig[32..64]\ns > SECP256K1_N_HALF?"}
B -- "yes (high-S, EIP-2 violation)" --> C["return Err(InvalidSignature)"]
B -- "no (low-S, EIP-2 compliant)" --> D["Extract sig_bytes[0..64] + recid = sig[64]"]
D --> E["secp256k1_ecdsa_address_recover_c(FFI)"]
E -- "ret == 0" --> F["Address::from_slice(output[12..])"]
E -- "ret != 0" --> G["return Err(RecoveryFailed)"]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/guest-program/src/crypto/zisk.rs
Line: 97-104
Comment:
**Duplicated `SECP256K1_N_HALF` constant**
`SECP256K1_N_HALF` is now defined identically in both `zisk.rs` and `shared.rs`. Since `shared.rs` already exports crypto helpers used by multiple backends, the constant could be promoted to a `pub(crate) const` there (or in a dedicated constants module) and imported here to keep a single source of truth.
```rust
// In shared.rs — promote to pub(crate)
pub(crate) const SECP256K1_N_HALF: [u8; 32] = [ … ];
// In zisk.rs — import instead of re-declaring
use super::shared::SECP256K1_N_HALF;
```
Not a blocking issue — both copies have the correct value — but a future value change (e.g. different curve) would need to be updated in two places.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add EIP-2 low-S signature validation to ..." | Re-trigger Greptile
🤖 Codex Code ReviewNo blocking findings.
Residual gap: I only found the high- Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const SECP256K1_N_HALF: [u8; 32] = [ | ||
| 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, | ||
| 0xff, 0xff, 0x5d, 0x57, 0x6e, 0x73, 0x57, 0xa4, 0x50, 0x1d, 0xdf, 0xe9, 0x2f, 0x46, | ||
| 0x68, 0x1b, 0x20, 0xa0, | ||
| ]; | ||
| #[allow(clippy::indexing_slicing)] | ||
| if sig[32..64] > SECP256K1_N_HALF[..] { | ||
| return Err(CryptoError::InvalidSignature); |
There was a problem hiding this comment.
SECP256K1_N_HALF constant
SECP256K1_N_HALF is now defined identically in both zisk.rs and shared.rs. Since shared.rs already exports crypto helpers used by multiple backends, the constant could be promoted to a pub(crate) const there (or in a dedicated constants module) and imported here to keep a single source of truth.
// In shared.rs — promote to pub(crate)
pub(crate) const SECP256K1_N_HALF: [u8; 32] = [ … ];
// In zisk.rs — import instead of re-declaring
use super::shared::SECP256K1_N_HALF;Not a blocking issue — both copies have the correct value — but a future value change (e.g. different curve) would need to be updated in two places.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/guest-program/src/crypto/zisk.rs
Line: 97-104
Comment:
**Duplicated `SECP256K1_N_HALF` constant**
`SECP256K1_N_HALF` is now defined identically in both `zisk.rs` and `shared.rs`. Since `shared.rs` already exports crypto helpers used by multiple backends, the constant could be promoted to a `pub(crate) const` there (or in a dedicated constants module) and imported here to keep a single source of truth.
```rust
// In shared.rs — promote to pub(crate)
pub(crate) const SECP256K1_N_HALF: [u8; 32] = [ … ];
// In zisk.rs — import instead of re-declaring
use super::shared::SECP256K1_N_HALF;
```
Not a blocking issue — both copies have the correct value — but a future value change (e.g. different curve) would need to be updated in two places.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewPR Review:
|
There was a problem hiding this comment.
Pull request overview
Adds EIP-2 low‑S signature enforcement to the ZisK guest crypto backend so it matches the other zkVM backends’ transaction/authority signature validation behavior, while keeping ecrecover behavior unchanged.
Changes:
- Add an EIP-2 low‑S (
s <= secp256k1n/2) check toZiskCrypto::recover_signerbefore calling the ZisK FFI recover routine. - Update the
ZiskCryptodocstring to reflect the current FFI-based implementation (instead of the old patched-crates approach).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn recover_signer(&self, sig: &[u8; 65], msg: &[u8; 32]) -> Result<Address, CryptoError> { | ||
| // EIP-2: reject high-s signatures (s > secp256k1n/2) | ||
| const SECP256K1_N_HALF: [u8; 32] = [ | ||
| 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, | ||
| 0xff, 0xff, 0x5d, 0x57, 0x6e, 0x73, 0x57, 0xa4, 0x50, 0x1d, 0xdf, 0xe9, 0x2f, 0x46, | ||
| 0x68, 0x1b, 0x20, 0xa0, | ||
| ]; | ||
| #[allow(clippy::indexing_slicing)] | ||
| if sig[32..64] > SECP256K1_N_HALF[..] { | ||
| return Err(CryptoError::InvalidSignature); | ||
| } | ||
|
|
There was a problem hiding this comment.
Crypto already provides a default recover_signer implementation (with the same EIP-2 low-S check) in crates/common/crypto/provider.rs. Keeping a custom implementation here duplicates the SECP256K1_N_HALF constant (now in at least 3 places) and increases the chance of future drift. Consider removing this recover_signer override and relying on the trait default (it will call this type’s secp256k1_ecrecover), or alternatively centralize the constant in one shared location.
Motivation
In #6414, ZisK's crypto was rewritten to use native FFI accelerators. A review concern identified that
recover_signer(used for transaction signature verification and EIP-7702 authority recovery) passes signatures directly to the C FFI function without enforcing the EIP-2 low-S check. This means ZisK would accept high-S (malleable) transaction signatures that all other backends (SP1, RISC0, OpenVM) correctly reject.ZisK's FFI
secp256k1_ecdsa_recovervalidates thatsis in[1, N)but does not enforce the stricter EIP-2 requirement ofs <= N/2.The
ecrecoverprecompile is intentionally left without this check, matching Ethereum spec behavior.Description
Add the same
SECP256K1_N_HALFvalidation to ZisK'srecover_signerthat already exists inshared.rs:k256_recover_signerfor the other backends. Signatures withs > secp256k1n/2now returnCryptoError::InvalidSignaturebefore reaching the FFI call.Also updates the stale
ZiskCryptodocstring that still referenced the old k256/substrate-bn patched crate approach.Closes #6432
Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync. — N/A