Skip to content

fix(l2): add EIP-2 low-S signature validation to ZisK's recover_signer #6469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
avilagaston9 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(l2): add EIP-2 low-S signature validation to ZisK's recover_signer #6469

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee90082
Add EIP-2 low-S signature validation to ZisK's recover_signer
avilagaston9 Apr 13, 2026
76b1c9e
Merge branch 'main' into fix/zisk-eip2-low-s-validation
avilagaston9 Apr 14, 2026
e6cd279
Merge branch 'main' into fix/zisk-eip2-low-s-validation
avilagaston9 Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions crates/guest-program/src/crypto/zisk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,8 @@ unsafe extern "C" {

/// ZisK crypto provider.
///
/// Uses k256 for ECDSA (secp256k1) and substrate-bn for all BN254 operations.
/// Overrides `mulmod256` and `modexp` with ZisK's native circuit instructions via `ziskos`.
///
/// When building actual ZisK guest binaries, ZisK's patched crate versions
/// of k256 and substrate-bn are used transparently via Cargo patches.
/// Uses ZisK's native FFI crypto accelerators (keccak256, SHA-256, ECDSA secp256k1/r1,
/// BN254, BLS12-381, modexp, blake2, KZG) exposed via `ziskos`.
#[derive(Debug)]
pub struct ZiskCrypto;

Expand All @@ -96,6 +93,17 @@ impl Crypto for ZiskCrypto {
}

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);
Comment on lines +97 to +104
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 13, 2026

Choose a reason for hiding this comment

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

P2 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.

// 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.

}

Comment on lines 95 to +106
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
// Extract signature (first 64 bytes) and recovery id (last byte)
let mut sig_bytes = [0u8; 64];
sig_bytes.copy_from_slice(&sig[..64]);
Expand Down
Loading