From 29a7702b44420cc19edbd147bff9994b0d5acd5e Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 29 Feb 2024 15:15:46 +0100 Subject: [PATCH 1/5] feat: add toCaipChainId helper + tests --- src/caip.test.ts | 46 +++++++++++++++++++++++++++++++++++++++++++ src/caip.ts | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 src/caip.test.ts create mode 100644 src/caip.ts diff --git a/src/caip.test.ts b/src/caip.test.ts new file mode 100644 index 00000000..8da4bbd4 --- /dev/null +++ b/src/caip.test.ts @@ -0,0 +1,46 @@ +import { CAIP_NAMESPACE_REGEX, CAIP_REFERENCE_REGEX } from '@metamask/utils'; + +import { toCaipChainId, CaipNamespaces } from './caip'; + +describe('toCaipChainId', () => { + // This function relies on @metamask/utils CAIP helpers. Those are being + // tested with a variety of inputs. + // Here we mainly focus on our own wrapper around those: + + it('returns a valid CAIP-2 chain ID', () => { + const namespace = CaipNamespaces.Eip155; + const reference = '1'; + expect(toCaipChainId(namespace, reference)).toBe( + `${namespace}:${reference}`, + ); + }); + + it.each([ + // Too short, must have 3 chars at least + '', + 'xs', + // Not matching + '!@#$%^&*()', + // Too long + 'namespacetoolong', + ])('throws for invalid namespaces: %s', (namespace) => { + const reference = '1'; + expect(() => toCaipChainId(namespace, reference)).toThrow( + `Invalid "namespace", must match: ${CAIP_NAMESPACE_REGEX.toString()}`, + ); + }); + + it.each([ + // Too short, must have 1 char at least + '', + // Not matching + '!@#$%^&*()', + // Too long + '012345678901234567890123456789012', // 33 chars + ])('throws for invalid reference: %s', (reference) => { + const namespace = CaipNamespaces.Eip155; + expect(() => toCaipChainId(namespace, reference)).toThrow( + `Invalid "reference", must match: ${CAIP_REFERENCE_REGEX.toString()}`, + ); + }); +}); diff --git a/src/caip.ts b/src/caip.ts new file mode 100644 index 00000000..127664f2 --- /dev/null +++ b/src/caip.ts @@ -0,0 +1,51 @@ +import { + CAIP_NAMESPACE_REGEX, + CAIP_REFERENCE_REGEX, + isCaipNamespace, + isCaipReference, +} from '@metamask/utils'; +import type { + CaipNamespace, + CaipReference, + CaipChainId, +} from '@metamask/utils'; + +/** Supported CAIP namespaces. */ +export const CaipNamespaces = { + /** Namespace for EIP-155 compatible chains. */ + Eip155: 'eip155' as CaipNamespace, +} as const; + +/** + * Chain ID as defined per the CAIP-2 + * {@link https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md}. + * + * It defines a way to uniquely identify any blockchain in a human-readable + * way. + * + * @param namespace - The standard (ecosystem) of similar blockchains. + * @param reference - Identify of a blockchain within a given namespace. + * @throws {@link Error} + * This exception is thrown if the inputs does not comply with the CAIP-2 + * syntax specification + * {@link https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md#syntax}. + * @returns A CAIP chain ID. + */ +export function toCaipChainId( + namespace: CaipNamespace, + reference: CaipReference, +): CaipChainId { + if (!isCaipNamespace(namespace)) { + throw new Error( + `Invalid "namespace", must match: ${CAIP_NAMESPACE_REGEX.toString()}`, + ); + } + + if (!isCaipReference(reference)) { + throw new Error( + `Invalid "reference", must match: ${CAIP_REFERENCE_REGEX.toString()}`, + ); + } + + return `${namespace}:${reference}`; +} From f90d6c0befe92bbb68074ed042dbf2af93ea1ed2 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 29 Feb 2024 15:17:00 +0100 Subject: [PATCH 2/5] feat: compute caip-2 chain ID when submitting requests --- src/SnapKeyring.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/SnapKeyring.ts b/src/SnapKeyring.ts index 5e72c60d..0033e3ab 100644 --- a/src/SnapKeyring.ts +++ b/src/SnapKeyring.ts @@ -30,6 +30,7 @@ import { EventEmitter } from 'events'; import { assert, mask, object, string } from 'superstruct'; import { v4 as uuid } from 'uuid'; +import { toCaipChainId, CaipNamespaces } from './caip'; import { DeferredPromise } from './DeferredPromise'; import { KeyringSnapControllerClient } from './KeyringSnapControllerClient'; import { SnapIdMap } from './SnapIdMap'; @@ -454,17 +455,19 @@ export class SnapKeyring extends EventEmitter { transaction: TypedTransaction, _opts = {}, ): Promise { + const chainId = transaction.common.chainId(); const tx = toJson({ ...transaction.toJSON(), from: address, type: `0x${transaction.type.toString(16)}`, - chainId: bigIntToHex(transaction.common.chainId()), + chainId: bigIntToHex(chainId), }); const signedTx = await this.#submitRequest({ address, method: EthMethod.SignTransaction, params: [tx], + chainId: toCaipChainId(CaipNamespaces.Eip155, `${chainId}`), }); // ! It's *** CRITICAL *** that we mask the signature here, otherwise the @@ -509,11 +512,18 @@ export class SnapKeyring extends EventEmitter { // used if the version is not specified or not supported. const method = methods[opts.version] || EthMethod.SignTypedDataV1; + // Extract chain ID as if it was a typed message (as defined by EIP-712), if + // input is not a typed message, then chain ID will be undefined! + const chainId = (data as TypedMessage).domain?.chainId; + return strictMask( await this.#submitRequest({ address, method, params: toJson([address, data]), + ...(chainId === undefined + ? {} + : { chainId: toCaipChainId(CaipNamespaces.Eip155, `${chainId}`) }), }), EthBytesStruct, ); From f649ff86b4e56cd5d7f049218baa4da8fbba8e5b Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 29 Feb 2024 16:22:26 +0100 Subject: [PATCH 3/5] test: verify signTransaction call parameters For some reason, the encoding of `type` and `data` are different from what we have. I didn't track down this behavior, so it could be a small "mistake" on our side, or there is different encoding being used somewhere in the flow. --- src/SnapKeyring.test.ts | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index febd48fc..885aaf3a 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -540,14 +540,14 @@ describe('SnapKeyring', () => { describe('signTransaction', () => { it('signs a ethereum transaction synchronously', async () => { const mockTx = { - data: '0x0', + data: '0x00', gasLimit: '0x26259fe', gasPrice: '0x1', nonce: '0xfffffffe', to: '0xccccccccccccd000000000000000000000000000', value: '0x1869e', chainId: '0x1', - type: '0x00', + type: '0x0', }; const mockSignedTx = { ...mockTx, @@ -561,7 +561,32 @@ describe('SnapKeyring', () => { pending: false, result: mockSignedTx, }); + const signature = await keyring.signTransaction(accounts[0].address, tx); + expect(mockSnapController.handleRequest).toHaveBeenCalledWith({ + snapId, + handler: 'onKeyringRequest', + origin: 'metamask', + request: { + id: expect.any(String), + jsonrpc: '2.0', + method: 'keyring_submitRequest', + params: { + id: expect.any(String), + scope: expect.any(String), + account: accounts[0].id, + request: { + method: 'eth_signTransaction', + params: [ + { + ...mockTx, + from: accounts[0].address, + }, + ], + }, + }, + }, + }); expect(signature).toStrictEqual(expectedSignedTx); }); }); From fb591b071eb6d78f7ac94a5e18b178f4bdd7d665 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 29 Feb 2024 16:25:04 +0100 Subject: [PATCH 4/5] test: verify scope during sign{Transaction,TypedMessage} calls --- src/SnapKeyring.test.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index 885aaf3a..c9a9f77d 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -557,6 +557,8 @@ describe('SnapKeyring', () => { }; const tx = TransactionFactory.fromTxData(mockTx); const expectedSignedTx = TransactionFactory.fromTxData(mockSignedTx); + const expectedScope = 'eip155:1'; + mockSnapController.handleRequest.mockResolvedValue({ pending: false, result: mockSignedTx, @@ -573,7 +575,7 @@ describe('SnapKeyring', () => { method: 'keyring_submitRequest', params: { id: expect.any(String), - scope: expect.any(String), + scope: expectedScope, account: accounts[0].id, request: { method: 'eth_signTransaction', @@ -630,6 +632,7 @@ describe('SnapKeyring', () => { }, }; + const expectedScope = 'eip155:1'; const expectedSignature = '0x4355c47d63924e8a72e509b65029052eb6c299d53a04e167c5775fd466751c9d07299936d304c153f6443dfa05f40ff007d72911b6f72307f996231605b915621c'; @@ -653,7 +656,7 @@ describe('SnapKeyring', () => { method: 'keyring_submitRequest', params: { id: expect.any(String), - scope: expect.any(String), + scope: expectedScope, account: accounts[0].id, request: { method: 'eth_signTypedData_v1', @@ -686,7 +689,7 @@ describe('SnapKeyring', () => { method: 'keyring_submitRequest', params: { id: expect.any(String), - scope: expect.any(String), + scope: expectedScope, account: accounts[0].id, request: { method: 'eth_signTypedData_v4', @@ -719,7 +722,7 @@ describe('SnapKeyring', () => { method: 'keyring_submitRequest', params: { id: expect.any(String), - scope: expect.any(String), + scope: expectedScope, account: accounts[0].id, request: { method: 'eth_signTypedData_v1', From b09a4eaac3a93a275f9038d09813f0f504912dc8 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 29 Feb 2024 16:25:42 +0100 Subject: [PATCH 5/5] test: add test over scope with non-defined chainId --- src/SnapKeyring.test.ts | 47 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/SnapKeyring.test.ts b/src/SnapKeyring.test.ts index c9a9f77d..2791c562 100644 --- a/src/SnapKeyring.test.ts +++ b/src/SnapKeyring.test.ts @@ -734,6 +734,53 @@ describe('SnapKeyring', () => { expect(signature).toStrictEqual(expectedSignature); }); + it('signs typed data without domain chainId has no scope', async () => { + mockSnapController.handleRequest.mockResolvedValue({ + pending: false, + result: expectedSignature, + }); + + const dataToSignWithoutDomainChainId = { + ...dataToSign, + domain: { + name: dataToSign.domain.name, + version: dataToSign.domain.version, + verifyingContract: dataToSign.domain.verifyingContract, + // We do not defined the chainId (it's currently marked as + // optional in the current type declaration). + // chainId: 1, + }, + }; + + const signature = await keyring.signTypedData( + accounts[0].address, + dataToSignWithoutDomainChainId, + { version: SignTypedDataVersion.V4 }, + ); + expect(mockSnapController.handleRequest).toHaveBeenCalledWith({ + snapId, + handler: 'onKeyringRequest', + origin: 'metamask', + request: { + id: expect.any(String), + jsonrpc: '2.0', + method: 'keyring_submitRequest', + params: { + id: expect.any(String), + // Without chainId alongside the typed message, we cannot + // compute the scope for this request! + scope: '', // Default value for `signTypedTransaction` + account: accounts[0].id, + request: { + method: 'eth_signTypedData_v4', + params: [accounts[0].address, dataToSignWithoutDomainChainId], + }, + }, + }, + }); + expect(signature).toStrictEqual(expectedSignature); + }); + it('calls eth_prepareUserOperation', async () => { const baseTxs = [ {