Skip to content

fix(id): flip recovery id for normalized ECDSA signatures - #4036

Open
Kewe63 wants to merge 1 commit into
xmtp:mainfrom
Kewe63:fix-4035-ecdsa-recovery-id
Open

fix(id): flip recovery id for normalized ECDSA signatures#4036
Kewe63 wants to merge 1 commit into
xmtp:mainfrom
Kewe63:fix-4035-ecdsa-recovery-id

Conversation

@Kewe63

@Kewe63 Kewe63 commented Aug 24, 2026

Copy link
Copy Markdown

Summary

Fixes #4035

to_lower_s() normalized high-s ECDSA signatures to low-s form but kept the original recovery id byte unchanged.

For recoverable ECDSA signatures, changing s to its low-s equivalent also requires flipping the recovery id y-parity bit. Otherwise the normalized signature can recover a different Ethereum signer address.


Changes

  • Flip the recovery id y-parity when normalize_s() changes s.
  • Handle raw k256 recovery ids, Ethereum legacy 27/28, and EIP-155-style values.
  • Add a regression test that constructs a high-s equivalent signature and verifies normalization still recovers the original signer.

How to Test

cargo fmt -p xmtp_id --check
cargo test -p xmtp_id test_to_lower_s_preserves_recoverable_ecdsa_signer
cargo test -p xmtp_id test_to_lower_s
cargo test -p xmtp_id associations::signature::tests
cargo check -p xmtp_id
git diff --check

Note: cargo test -p xmtp_id was also attempted. It passed 39 tests and failed 3 SCW verifier tests because no local RPC/anvil service was listening on 127.0.0.1:8545. The failures appear unrelated to this signature normalization change.


Checklist

  • Tests pass — targeted signature normalization tests green (unrelated SCW verifier failures due to missing local RPC service)
  • cargo fmt clean
  • Follows Conventional Commits
  • Changes scoped to this fix only

Risk & Impact

Low-to-medium. The fix only changes behavior for high-s signatures that require normalization — the recovery id flip is applied exactly when normalize_s() changes s, so low-s signatures are unaffected. The regression test verifies the normalized signature still recovers the original signer address.

Type: 🐛 Bug fix
Fixes: #4035

Note

Fix to_lower_s to flip recovery id y-parity for normalized ECDSA signatures

  • When to_lower_s normalizes a high-s ECDSA signature to low-s, it now flips the appended recovery id using the new flip_recovery_id_y_parity helper so the correct signer can still be recovered.
  • The helper supports raw k256 ids (0..=3), Ethereum legacy v (27/28), and EIP-155 v (>=35), returning SignatureError::Invalid on invalid inputs or arithmetic overflow.
  • If no normalization is needed, the original bytes and recovery id are preserved as before.
  • Risk: for certain EIP-155 values near u8::MAX, to_lower_s may now return SignatureError::Invalid due to overflow where it previously succeeded with an unmodified recovery id.

Macroscope summarized 72dbb7c.

@Kewe63
Kewe63 requested a review from a team as a code owner August 24, 2026 18:54
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.

[Bug]: xmtp_id high-s ECDSA normalization does not flip recovery id

1 participant