Skip to content

fix(crypto-transaction): bind legacy second signature to signed transaction - #1522

Merged
sebastijankuzner merged 3 commits into
developfrom
fix/crypto-transaction/bind-legacy-second-signature
Oct 9, 2026
Merged

sebastijankuzner merged 3 commits into
developfrom
fix/crypto-transaction/bind-legacy-second-signature

Conversation

@oXtxNt9U

@oXtxNt9U oXtxNt9U commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The legacy second signature (LSS) signed the same digest as the primary signature and covered neither the sender nor the primary signature. So an LSS was also a valid transaction from the second key's own address, it could be reused by other wallets sharing that second key, and any ordinary signature by the second key over the same body could be used as an LSS. Its last byte (the recovery id) was also never checked, so anyone could make 256 valid variants of an LSS under one transaction hash.

Fixes

  • Sign the LSS over keccak256(0x8f ‖ "MAINSAIL_LSS_V1" ‖ txHash), where txHash is the hash of the primary-signed transaction. This binds it to the sender and the primary signature, and the 0x8f prefix means it can never match a transaction or signed-message preimage. The builder now requires the primary signature first.
  • Verify the LSS by recovering the key and comparing it to the second key, with the recovery id limited to 0 or 1, so the last byte is bound.
  • Verification no longer re-serializes the transaction, so its cost no longer grows with payload size.

Tests

  • A regression test for each case: LSS replayed as a primary signature, signed over the unsigned transaction, reused by another sender, and an altered recovery id.
  • Updated test vectors, cross-checked with viem.

Note

LSS producers (TS, PHP and Python SDKs) must sign the new digest, where txHash is the standard hash of the 9-field signed transaction (minimal r/s, without the LSS), and emit a recovery id of 0 or 1.

Breaking

  • TransactionBuilder.legacySecondSign* must be called after sign* (throws MissingTransactionSignatureError otherwise).
  • legacySecondSign takes the signed transaction; verifyLegacySecondSignature takes TransactionData and uses its hash.

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.60%. Comparing base (c39bb16) to head (5cd4efc).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1522      +/-   ##
===========================================
+ Coverage    80.31%   80.60%   +0.29%     
===========================================
  Files          952      963      +11     
  Lines        17162    17601     +439     
  Branches      2599     2678      +79     
===========================================
+ Hits         13784    14188     +404     
- Misses        3373     3405      +32     
- Partials         5        8       +3     
Flag Coverage Δ
contracts 91.95% <ø> (?)
packages 80.32% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@oXtxNt9U
oXtxNt9U force-pushed the fix/crypto-transaction/bind-legacy-second-signature branch from d6520e8 to 2f6387c Compare October 7, 2026 03:32
@sebastijankuzner
sebastijankuzner merged commit 9a9454a into develop Oct 9, 2026
65 of 68 checks passed
@sebastijankuzner
sebastijankuzner deleted the fix/crypto-transaction/bind-legacy-second-signature branch October 9, 2026 13:14
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