fix: canonicalize typed-data signature v-byte before forwarding on-chain (Permit2 flows failing for Frame/Ledger wallets) - #1147
Open
tjcloa wants to merge 2 commits into
Open
Conversation
Wallets that expose raw signer output (Frame, onboard-ledger, some WalletConnect/MPC wallets) return the ECDSA recovery byte as 0/1. ethers' verifyTypedData tolerates that form, so the dapp's own post-signing check passed, but Permit2's SignatureVerification feeds v straight into ecrecover, which returns the zero address for v=0/1 and reverts with InvalidSignature (0x8baa579f). This broke every Permit2 flow (DLLR trove close/adjust/repay, stability pool deposits, DLLR conversion) for affected wallets — e.g. rsk tx 0xa53eef8b687f745c9b012f383310f3c38fc2a7e2113b371fa9f710c1050020b2, whose replay succeeds with only the v byte rewritten 0x00->0x1b. Normalize the signature at the single signing chokepoint (TransactionSteps) via joinSignature(splitSignature(...)): 0/1 maps to 27/28, 64-byte EIP-2098 compact expands to canonical 65 bytes, canonical signatures pass through byte-for-byte unchanged, malformed v throws at signing time instead of burning gas on-chain. Regression tests pin all four v-byte cases, the compact form, and the exact signature from the failed tx.
🦋 Changeset detectedLatest commit: 0bddb7a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for sovryn-dapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ure-v-normalization
There was a problem hiding this comment.
Pull request overview
This PR fixes Permit2 typed-data flows for wallets that return non-canonical EIP-712 signatures (notably v=0/1 and EIP-2098 compact signatures) by normalizing signatures at the frontend “typed-data signing” chokepoint before verification and on-chain forwarding.
Changes:
- Add
normalizeSignature()utility that canonicalizes signatures viajoinSignature(splitSignature(...)). - Normalize
_signTypedDataresults insideTransactionSteps.tsxbeforeverifyTypedDataand before storing/forwarding the signature. - Add regression tests covering
v=0/1,v=27/28, EIP-2098 compact, malformedv, and a real incident signature; include a changeset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/frontend/src/utils/signature.ts | Introduces a signature normalization helper for on-chain-compatible `r |
| apps/frontend/src/utils/signature.test.ts | Adds unit tests covering normalization behavior across wallet/signature variants and an incident reproduction. |
| apps/frontend/src/app/3_organisms/TransactionStepDialog/components/TransactionSteps/TransactionSteps.tsx | Applies signature normalization to typed-data signing flow before verification/storage/forwarding. |
| .changeset/violet-eagles-recover.md | Publishes the fix as a patch changeset for the frontend package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+18
| import { ethers } from 'ethers'; | ||
|
|
||
| /** | ||
| * Canonicalizes a wallet-returned ECDSA signature to the 65-byte r||s||v hex | ||
| * form with v ∈ {27, 28} — the only encoding contracts that feed v straight | ||
| * into ecrecover (e.g. Permit2's SignatureVerification) accept. | ||
| * | ||
| * Wallets that expose raw signer output (Frame, the onboard-ledger module, | ||
| * some WalletConnect/MPC wallets) return v as the recovery id (0/1), and | ||
| * EIP-2098 signers return a 64-byte compact form. ethers' own verification | ||
| * helpers silently tolerate all of these, so a signature must be normalized | ||
| * with this function before it is sent on-chain, not just verified. | ||
| * | ||
| * Canonical signatures pass through byte-for-byte unchanged; a malformed | ||
| * recovery byte throws here, at signing time, instead of reverting on-chain. | ||
| */ | ||
| export const normalizeSignature = (signature: ethers.BytesLike): string => | ||
| ethers.utils.joinSignature(ethers.utils.splitSignature(signature)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Wallets that expose raw signer output — Frame, the first-party
@sovryn/onboard-ledgermodule (which deliberately converts Ledger's v 27/28 down to 0/1), and some WalletConnect/MPC wallets — return EIP-712 signatures with the recovery byte as0/1instead of the canonical27/28.The dapp's post-signing check (
ethers.utils.verifyTypedData) silently tolerates that form, so signing appears to succeed — but the raw signature is then forwarded on-chain, where Permit2'sSignatureVerificationfeedsvstraight intoecrecover. Forv=0/1ecrecover returns the zero address and the tx reverts withInvalidSignature()(0x8baa579f).This breaks every Permit2 flow for affected wallets: Zero trove close/adjust/repay with DLLR, Stability Pool DLLR deposits, and the DLLR conversion route.
Real-world incident: RSK tx 0xa53eef8b…0020b2 —
closeNueTroveWithPermit2from a Frame wallet. Replaying it at its original block with only the final signature byte rewritten0x00 → 0x1bsucceeds, proving all other protocol conditions were valid. The Permit2 at the canonical CREATE2 address on Rootstock is byte-identical to Uniswap's Ethereum deployment (modulo chain-id immutable), so no contract change is applicable.Fix
All typed-data signatures pass through a single chokepoint (
TransactionSteps.tsx). Canonicalize there, before the signature is verified/stored/forwarded, via newnormalizeSignature()(utils/signature.ts=joinSignature(splitSignature(sig))):v = 0/1→27/28_signTypedDataroundtrip test)vthrows at signing time instead of burning gas on a doomed txTests
8 regression tests pin all four v-byte cases, the compact form, the standard-signer no-op, and the exact signature from the failed incident tx (r/s/v + typed data lifted from calldata) recovering the original sender. Frontend suite 60/60 green; tsc + eslint clean; changeset included.