feat(wallets): add approval signature validation per signer type#1847
feat(wallets): add approval signature validation per signer type#1847devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Original prompt from daniil.dovgal
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 2b6f3ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/wallets/src/utils/signature-validation.ts:45
The ERC-6492 magic suffix comparison is case-sensitive (all lowercase), but `isHex()` from viem accepts mixed-case hex strings (`/^0x[0-9a-fA-F]*$/`). A signature whose suffix bytes happen to be uppercase — e.g., produced by a library that returns `toUpperCase()` hex — would silently pass this check and only be caught by the byte-length guard a few lines later, producing a generic "N bytes" error instead of the specific "ERC-6492 wrapped" message that helps callers diagnose the real issue. Normalizing to lowercase before the check ensures the specific error is always surfaced.
```suggestion
if (signature.toLowerCase().endsWith(ERC_6492_MAGIC_SUFFIX)) {
```
### Issue 2 of 2
packages/wallets/src/utils/signature-validation.ts:109-111
**`registerSignatureValidator` silently overwrites built-in validators**
`Map.set` replaces any existing entry, so a caller can accidentally shadow core validators (e.g. `registerSignatureValidator("external-wallet", myValidator)`). The JSDoc says "Existing validators can be extended without modifying this module," implying the intent is additive, not override. Consider throwing if the key already exists in the built-in set, or at minimum logging a warning, so intentional overrides remain possible while accidental ones surface early.
Reviews (1): Last reviewed commit: "fix(wallets): use BigInt() instead of 0n..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| if (signature.endsWith(ERC_6492_MAGIC_SUFFIX)) { |
There was a problem hiding this comment.
isHex() from viem accepts mixed-case hex strings (/^0x[0-9a-fA-F]*$/). A signature whose suffix bytes happen to be uppercase — e.g., produced by a library that returns toUpperCase() hex — would silently pass this check and only be caught by the byte-length guard a few lines later, producing a generic "N bytes" error instead of the specific "ERC-6492 wrapped" message that helps callers diagnose the real issue. Normalizing to lowercase before the check ensures the specific error is always surfaced.
| if (signature.endsWith(ERC_6492_MAGIC_SUFFIX)) { | |
| if (signature.toLowerCase().endsWith(ERC_6492_MAGIC_SUFFIX)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/utils/signature-validation.ts
Line: 45
Comment:
The ERC-6492 magic suffix comparison is case-sensitive (all lowercase), but `isHex()` from viem accepts mixed-case hex strings (`/^0x[0-9a-fA-F]*$/`). A signature whose suffix bytes happen to be uppercase — e.g., produced by a library that returns `toUpperCase()` hex — would silently pass this check and only be caught by the byte-length guard a few lines later, producing a generic "N bytes" error instead of the specific "ERC-6492 wrapped" message that helps callers diagnose the real issue. Normalizing to lowercase before the check ensures the specific error is always surfaced.
```suggestion
if (signature.toLowerCase().endsWith(ERC_6492_MAGIC_SUFFIX)) {
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Easy there, cold-blooded one 🐍 — the ERC-6492 magic suffix is 6492649264926492... which is all digits (0-9). Digits don't have upper/lower case variants, so .toLowerCase() is a no-op here. After the isHex() guard, TypeScript narrows the signature to `0x${string}` with only valid hex chars — and since the suffix contains zero alphabetic hex characters (a-f/A-F), case sensitivity is irrelevant.
No change needed — the check is correct as-is. 🦎
| export function registerSignatureValidator(signerType: string, validator: SignatureValidator): void { | ||
| validatorRegistry.set(signerType, validator); | ||
| } |
There was a problem hiding this comment.
registerSignatureValidator silently overwrites built-in validators
Map.set replaces any existing entry, so a caller can accidentally shadow core validators (e.g. registerSignatureValidator("external-wallet", myValidator)). The JSDoc says "Existing validators can be extended without modifying this module," implying the intent is additive, not override. Consider throwing if the key already exists in the built-in set, or at minimum logging a warning, so intentional overrides remain possible while accidental ones surface early.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/utils/signature-validation.ts
Line: 109-111
Comment:
**`registerSignatureValidator` silently overwrites built-in validators**
`Map.set` replaces any existing entry, so a caller can accidentally shadow core validators (e.g. `registerSignatureValidator("external-wallet", myValidator)`). The JSDoc says "Existing validators can be extended without modifying this module," implying the intent is additive, not override. Consider throwing if the key already exists in the built-in set, or at minimum logging a warning, so intentional overrides remain possible while accidental ones surface early.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
The lizard brain raises a fair point here 🦎 — but this is an intentional design choice. registerSignatureValidator is a public SDK extension point. Allowing overrides gives SDK consumers the flexibility to replace built-in validators with custom ones for their use case (e.g., a stricter ECDSA validator).
The JSDoc "extended without modifying this module" refers to adding new signer types — not a guarantee against overriding existing ones. Throwing on existing keys would break the Open/Closed extensibility pattern since there's no separate overrideSignatureValidator API.
That said, a console.warn when overwriting a built-in could be reasonable as a DX improvement — but that's a design decision for the maintainers. Leaving as-is for now and deferring to the reviewer. 🐊
1 participant
Description
A third-party client used
EVMWallet.signMessage()to sign theuserOperationHashfor a transaction approval. For undeployed smart wallets,signMessage()routes through the Signature API, which wraps the result with ERC-6492 (~580 bytes). The on-chain ECDSA validator expects a 65-byte raw ECDSA signature — it cannot parse the ERC-6492 envelope and reverts withInvalidSignature(), which surfaces as AA23.The client assumption that
signMessage(userOpHash)produces a valid transaction approval signature is incorrect — the SDK exposeswallet.approve()/wallet.send()specifically for this purpose.This PR adds SDK-side validation inside
wallet.approve()before calling the API, catching invalid signatures early with clear error messages:external-wallet,server,email,phone): validates hex format viaisHex(), rejects ERC-6492-wrapped signatures, checks 64/65-byte length viasize(), and structurally parses 65-byte sigs viaparseSignature()device): validatesrandscomponents satisfy1 ≤ value < P256_ORDERpasskey): delegates to P256 validation, then checks metadata presenceapi-key: bypasses validation (auto-approved)registerSignatureValidator())Uses a generic
SignatureValidator<TApproval>interface with typed approval aliases (EcdsaApproval,P256Approval,PasskeyApproval) for strong internal typing while the registry dispatches via the baseApprovaltype.Files changed:
packages/wallets/src/utils/signature-validation.ts— new validation modulepackages/wallets/src/utils/signature-validation.test.ts— 45 test casespackages/wallets/src/utils/errors.ts—InvalidSignatureForApprovalErrorpackages/wallets/src/wallets/wallet.ts— integration atapproveTransactionInternalandapproveSignatureInternalTest plan
{r,s}, just below curve order, r=0, s=0, r≥ORDER, s≥ORDER{r,s}registerSignatureValidatorPackage updates
@crossmint/wallets-sdk(patch): SDK-side approval signature validationLink to Devin session: https://crossmint.devinenterprise.com/sessions/5a13e5886299434b8ad992bb74a47901