Skip to content

fix: 7702 authorization/legacy-tx signature encoding and Simple7702 revoke guard - #220

Merged
Sednaoui merged 5 commits into
devfrom
fix/7702-signature-encoding
Jul 23, 2026
Merged

fix: 7702 authorization/legacy-tx signature encoding and Simple7702 revoke guard#220
Sednaoui merged 5 commits into
devfrom
fix/7702-signature-encoding

Conversation

@sherifahmed990

@sherifahmed990 sherifahmed990 commented Jul 22, 2026

Copy link
Copy Markdown
Member

Signature-encoding fixes on the EIP-7702 paths.

  • Legacy transactions with leading-zero r/s were rejected by nodes: createAndSignLegacyRawTransaction encoded r and s from the 32-byte zero-padded hex, so any signature whose
    r or s has a leading zero byte (~0.8% of transactions, deterministic per key/payload under RFC 6979) produced rlp: non-canonical integer (leading zero bytes). Both are now encoded
    as minimal RLP integers via bigintToBytes, like the EIP-7702 path.
  • High-s external signatures silently never applied delegation: EIP-7702 requires s <= n/2 in authorization tuples, and nodes silently skip high-s tuples during set-code processing — a signer callback returning a standard high-s signature failed with no error surfaced. parseRawSignature now normalizes to the complementary low-s form (s' = n − s,
    flipped parity), matching the private-key path.
  • Simple7702 revoke could revoke the wrong account's delegation: createRevokeDelegationTransaction fetched delegation status and the nonce for accountAddress but signed with
    eoaPrivateKey unconditionally — with a mismatched key, the broadcast transaction revoked the signer's delegation instead. The same up-front key/address check the Calibur revoke
    path has is now applied.

Also moves the secp256k1 constants above parseRawSignature for read-before-use ordering (no behavior change).

Summary by CodeRabbit

  • Bug Fixes

    • Prevented delegation revocation when the supplied private key does not match the account address.
    • Improved transaction signature encoding to use canonical, minimal formats.
    • Normalized high-s signatures for EIP-7702 authorizations, improving compatibility and validation.
  • Tests

    • Added coverage for canonical signature encoding and low-s normalization.

sherifahmed990 and others added 5 commits July 22, 2026 12:40
createAndSignLegacyRawTransaction encoded r and s via getBytes on the
32-byte zero-padded hex from the signer, so any signature whose r or s
has a leading zero byte (~0.8% of transactions, deterministic per
key/payload under RFC 6979) produced a raw transaction that nodes reject
with 'rlp: non-canonical integer (leading zero bytes)'. Encode both
through bigintToBytes like the EIP-7702 path already does.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EIP-7702 requires s <= n/2 in authorization tuples; nodes silently skip
high-s tuples during set-code processing, so a signer callback returning
a standard high-s signature produced an authorization that never applied
the delegation with no error surfaced. parseRawSignature now converts to
the complementary low-s signature (s' = n - s, flipped parity), matching
the private-key path which already signs with lowS.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
createRevokeDelegationTransaction fetched delegation status and the tx
nonce for accountAddress but signed with eoaPrivateKey unconditionally.
With a mismatched key the broadcast transaction's sender is the other
EOA, revoking the signer's delegation instead of the account the method
checked and reported on. Add the same up-front key/address check the
Calibur revoke path already has.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Read-before-use ordering; no behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…wing

Link the EIP-2 s-value bound and malleability rationale and the
EIP-7702 per-tuple validation/skip behavior, and note that generic
ECDSA signers (KMS/HSM/WebCrypto) produce high-s by design, so
rejection would fail nondeterministically on valid signatures.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change canonicalizes legacy ECDSA signature encoding, normalizes EIP-7702 signatures to low-s values, adds coverage for both behaviors, and validates that revoke-delegation private keys match the account address.

Changes

Signature and delegation correctness

Layer / File(s) Summary
Canonical legacy signature encoding
src/utils7702.ts, test/utils7702.test.js
Legacy transaction signatures use minimal big-endian RLP integers, with tests verifying encoding and sender recovery.
EIP-7702 low-s normalization
src/utils7702.ts, test/utils7702.test.js
Standard and compact signatures normalize high-s values and flip parity, with authorization tests covering the result.
Revoke-delegation signer validation
src/account/simple/Simple7702Account.ts
Revoke-delegation creation rejects private keys whose derived address differs from the configured account address.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: sednaoui, andrewwahid

Poem

I’m a rabbit with signatures neat,
Low-s hops make the encoding complete.
Keys now match the account they defend,
High-s flips to its canonical friend.
RLP sheds each needless byte—
Hoppy correctness, sealed tight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the main changes, but it does not follow the required Summary/Test/Risk / Compatibility template. Add the required ## Summary, ## Test, and ## Risk / Compatibility sections, and include test evidence plus any compatibility or rollout notes.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: 7702 signature encoding fixes and the Simple7702 revoke guard.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/utils7702.ts (1)

483-506: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Low-s normalization logic is correct; consider bounding r/s before trusting external signer output.

The flip (s = n - s, yParity = 1 - yParity) and the strict s > SECP256K1_HALF_N boundary correctly implement the EIP-2/EIP-7702 canonicalization rule. One gap: r and the pre-normalization s from an external signer callback are never checked against (0, n); a malformed compact/standard signature (e.g. s >= n) would still be "normalized" and returned as a well-formed-looking tuple instead of being rejected early, deferring the failure to silent on-chain tuple-skipping described in the docstring.

Optional validation
 	const r = BigInt(`0x${sig.slice(0, 64)}`);
+	if (r <= 0n || r >= SECP256K1_N) {
+		throw new RangeError(`invalid signature r value: out of range`);
+	}
 	let yParity: 0 | 1;
 	let s: bigint;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils7702.ts` around lines 483 - 506, Validate the parsed r and
pre-normalization s values in the signature parsing flow before applying low-s
normalization: both must be greater than zero and less than SECP256K1_N,
otherwise reject the external signer output. Preserve the existing yParity
handling and complementary low-s flip for valid values, using the parsed r
symbol alongside s.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/utils7702.ts`:
- Around line 483-506: Validate the parsed r and pre-normalization s values in
the signature parsing flow before applying low-s normalization: both must be
greater than zero and less than SECP256K1_N, otherwise reject the external
signer output. Preserve the existing yParity handling and complementary low-s
flip for valid values, using the parsed r symbol alongside s.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fcf52971-4b18-40ea-acfa-b66f57c6d555

📥 Commits

Reviewing files that changed from the base of the PR and between a4f98c7 and a2fd5ac.

📒 Files selected for processing (3)
  • src/account/simple/Simple7702Account.ts
  • src/utils7702.ts
  • test/utils7702.test.js

@Sednaoui

Copy link
Copy Markdown
Member

LGTM

@Sednaoui
Sednaoui merged commit 786d840 into dev Jul 23, 2026
3 checks passed
@Sednaoui
Sednaoui deleted the fix/7702-signature-encoding branch July 23, 2026 13:37
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.

2 participants