From 4ef39742d255340e7095ff168096b66197e1a2a9 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Thu, 18 Jul 2024 20:36:38 +0530 Subject: [PATCH 1/4] Changes in typed signature validation and normalization --- src/utils/normalize.test.ts | 32 ++------- src/utils/normalize.ts | 36 ++-------- src/wallet.test.ts | 135 +++++++++++++++++++++++++++++++++++- src/wallet.ts | 29 ++++++-- 4 files changed, 167 insertions(+), 65 deletions(-) diff --git a/src/utils/normalize.test.ts b/src/utils/normalize.test.ts index 8c743a4c..77bb42e0 100644 --- a/src/utils/normalize.test.ts +++ b/src/utils/normalize.test.ts @@ -47,7 +47,7 @@ const MESSAGE_DATA_MOCK = { name: 'Liquid staked Ether 2.0', version: '2', chainId: '0x1', - verifyingContract: '996101235222674412020337938588541139382869425796', + verifyingContract: '0xae7ab96520de3a18e5e111b5eaab095312d7fe84', }, primaryType: 'Permit', message: { @@ -66,38 +66,16 @@ describe('normalizeTypedMessage', () => { } it('should normalize verifyingContract address in domain', () => { - const normalizedData = parseNormalizerResult(MESSAGE_DATA_MOCK); - expect(normalizedData.domain.verifyingContract).toBe( - '0xae7ab96520de3a18e5e111b5eaab095312d7fe84', - ); - }); - - it('should normalize verifyingContract address in domain when provided data is an object', () => { - const NON_STRINGIFIED_MESSAGE_DATA_MOCK = MESSAGE_DATA_MOCK; - const normalizedData = JSON.parse( - normalizeTypedMessage( - NON_STRINGIFIED_MESSAGE_DATA_MOCK as unknown as string, - ), - ); - expect(normalizedData.domain.verifyingContract).toBe( - '0xae7ab96520de3a18e5e111b5eaab095312d7fe84', - ); - }); - - it('should handle octal verifyingContract address by normalizing it', () => { - const expectedNormalizedOctalAddress = '0x53'; - const messageDataWithOctalAddress = { + const msg_mock = { ...MESSAGE_DATA_MOCK, domain: { ...MESSAGE_DATA_MOCK.domain, - verifyingContract: '0o123', + verifyingContract: '0Xae7ab96520de3a18e5e111b5eaab095312d7fe84', }, }; - - const normalizedData = parseNormalizerResult(messageDataWithOctalAddress); - + const normalizedData = parseNormalizerResult(msg_mock); expect(normalizedData.domain.verifyingContract).toBe( - expectedNormalizedOctalAddress, + '0xae7ab96520de3a18e5e111b5eaab095312d7fe84', ); }); diff --git a/src/utils/normalize.ts b/src/utils/normalize.ts index f063e423..7637cbc9 100644 --- a/src/utils/normalize.ts +++ b/src/utils/normalize.ts @@ -1,9 +1,7 @@ -import { add0x, isValidHexAddress, isStrictHexString } from '@metamask/utils'; import type { Hex } from '@metamask/utils'; -import BN from 'bn.js'; type EIP712Domain = { - verifyingContract: string; + verifyingContract: Hex; }; type SignTypedMessageDataV3V4 = { @@ -45,7 +43,7 @@ export function normalizeTypedMessage(messageData: string) { * @param data - The messageData to parse. * @returns The data object for EIP712 normalization. */ -function parseTypedMessage(data: string) { +export function parseTypedMessage(data: string) { if (typeof data !== 'string') { return data; } @@ -54,36 +52,14 @@ function parseTypedMessage(data: string) { } /** - * Normalizes the address to a hexadecimal format + * Normalizes the address to standard hexadecimal format * * @param address - The address to normalize. * @returns The normalized address. */ -function normalizeContractAddress(address: string): Hex | string { - if (isStrictHexString(address) && isValidHexAddress(address)) { - return address; +function normalizeContractAddress(address: Hex): Hex { + if (address.startsWith('0X')) { + return `0x${address.slice(2)}`; } - - // Check if the address is in octal format, convert to hexadecimal - if (address.startsWith('0o')) { - // If octal, convert to hexadecimal - return octalToHex(address); - } - - // Check if the address is in decimal format, convert to hexadecimal - try { - const decimalBN = new BN(address, 10); - const hexString = decimalBN.toString(16); - return add0x(hexString); - } catch (e) { - // Ignore errors and return the original address - } - - // Returning the original address without normalization return address; } - -function octalToHex(octalAddress: string): Hex { - const decimalAddress = parseInt(octalAddress.slice(2), 8).toString(16); - return add0x(decimalAddress); -} diff --git a/src/wallet.test.ts b/src/wallet.test.ts index eaf29acb..9a8e783c 100644 --- a/src/wallet.test.ts +++ b/src/wallet.test.ts @@ -357,7 +357,7 @@ describe('wallet', () => { }, primaryType: 'EIP712Domain', domain: { - verifyingContract: '996101235222674412020337938588541139382869425796', + verifyingContract: '0Xae7ab96520de3a18e5e111b5eaab095312d7fe84', }, message: {}, }; @@ -391,6 +391,139 @@ describe('wallet', () => { signatureMethod: 'eth_signTypedData_v3', }); }); + + it('should throw if verifyingContract is invalid hex value', async () => { + const { engine } = createTestSetup(); + const getAccounts = async () => testAddresses.slice(); + const witnessedMsgParams: TypedMessageParams[] = []; + const processTypedMessageV3 = async (msgParams: TypedMessageParams) => { + witnessedMsgParams.push(msgParams); + // Assume testMsgSig is the expected signature result + return testMsgSig; + }; + + engine.push( + createWalletMiddleware({ getAccounts, processTypedMessageV3 }), + ); + + const message = { + types: { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'version', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContract', type: 'address' }, + ], + }, + primaryType: 'EIP712Domain', + domain: { + verifyingContract: '917551056842671309452305380979543736893630245704', + }, + message: {}, + }; + + const stringifiedMessage = JSON.stringify(message); + + const payload = { + method: 'eth_signTypedData_v3', + params: [testAddresses[0], stringifiedMessage], // Assuming testAddresses[0] is a valid address from your setup + }; + + const promise = pify(engine.handle).call(engine, payload); + await expect(promise).rejects.toThrow('Invalid input.'); + }); + }); + + describe('signTypedDataV4', () => { + const getMsgParams = (verifyingContract?: string) => ({ + types: { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'version', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContract', type: 'address' }, + ], + Permit: [ + { name: 'owner', type: 'address' }, + { name: 'spender', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'deadline', type: 'uint256' }, + ], + }, + primaryType: 'Permit', + domain: { + name: 'MyToken', + version: '1', + verifyingContract: + verifyingContract ?? '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC', + chainId: '0x1', + }, + message: { + owner: testAddresses[0], + spender: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc', + value: 3000, + nonce: 0, + deadline: 50000000000, + }, + }); + + it('should not throw if request is permit with valid hex value for verifyingContract address', async () => { + const { engine } = createTestSetup(); + const getAccounts = async () => testAddresses.slice(); + const witnessedMsgParams: TypedMessageParams[] = []; + const processTypedMessageV4 = async (msgParams: TypedMessageParams) => { + witnessedMsgParams.push(msgParams); + // Assume testMsgSig is the expected signature result + return testMsgSig; + }; + + engine.push( + createWalletMiddleware({ getAccounts, processTypedMessageV4 }), + ); + + const payload = { + method: 'eth_signTypedData_v4', + params: [testAddresses[0], JSON.stringify(getMsgParams())], + }; + + const promise = pify(engine.handle).call(engine, payload); + const result = await promise; + expect(result).toStrictEqual({ + id: undefined, + jsonrpc: undefined, + result: + '0x68dc980608bceb5f99f691e62c32caccaee05317309015e9454eba1a14c3cd4505d1dd098b8339801239c9bcaac3c4df95569dcf307108b92f68711379be14d81c', + }); + }); + + it('should throw if request is permit with invalid hex value for verifyingContract address', async () => { + const { engine } = createTestSetup(); + const getAccounts = async () => testAddresses.slice(); + const witnessedMsgParams: TypedMessageParams[] = []; + const processTypedMessageV4 = async (msgParams: TypedMessageParams) => { + witnessedMsgParams.push(msgParams); + // Assume testMsgSig is the expected signature result + return testMsgSig; + }; + + engine.push( + createWalletMiddleware({ getAccounts, processTypedMessageV4 }), + ); + + const payload = { + method: 'eth_signTypedData_v4', + params: [ + testAddresses[0], + JSON.stringify( + getMsgParams('917551056842671309452305380979543736893630245704'), + ), + ], + }; + + const promise = pify(engine.handle).call(engine, payload); + await expect(promise).rejects.toThrow('Invalid input.'); + }); }); describe('sign', () => { diff --git a/src/wallet.ts b/src/wallet.ts index 58c8bb4d..d408eb66 100644 --- a/src/wallet.ts +++ b/src/wallet.ts @@ -5,14 +5,15 @@ import { createScaffoldMiddleware, } from '@metamask/json-rpc-engine'; import { providerErrors, rpcErrors } from '@metamask/rpc-errors'; -import type { - Json, - JsonRpcRequest, - PendingJsonRpcResponse, +import { + isValidHexAddress, + type Json, + type JsonRpcRequest, + type PendingJsonRpcResponse, } from '@metamask/utils'; import type { Block } from './types'; -import { normalizeTypedMessage } from './utils/normalize'; +import { normalizeTypedMessage, parseTypedMessage } from './utils/normalize'; /* export type TransactionParams = { @@ -275,9 +276,9 @@ WalletMiddlewareOptions): JsonRpcMiddleware { } const params = req.params as [string, string]; - const address = await validateAndNormalizeKeyholder(params[0], req); const message = normalizeTypedMessage(params[1]); + validateVerifyingContract(message); const version = 'V3'; const msgParams: TypedMessageParams = { data: message, @@ -305,9 +306,9 @@ WalletMiddlewareOptions): JsonRpcMiddleware { } const params = req.params as [string, string]; - const address = await validateAndNormalizeKeyholder(params[0], req); const message = normalizeTypedMessage(params[1]); + validateVerifyingContract(message); const version = 'V4'; const msgParams: TypedMessageParams = { data: message, @@ -490,6 +491,20 @@ WalletMiddlewareOptions): JsonRpcMiddleware { } } +/** + * Validates verifyingContract of typedSignMessage. + * + * @param data - The data passed in typedSign request. + */ +function validateVerifyingContract(data: string) { + const { + domain: { verifyingContract }, + } = parseTypedMessage(data); + if (!isValidHexAddress(verifyingContract)) { + throw rpcErrors.invalidInput(); + } +} + function resemblesAddress(str: string): boolean { // hex prefix 2 + 20 bytes return str.length === 2 + 20 * 2; From bd6b77e6db494bab7fce757028861016902053e1 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Thu, 18 Jul 2024 21:13:26 +0530 Subject: [PATCH 2/4] fix --- src/wallet.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/wallet.ts b/src/wallet.ts index d408eb66..db8eee6c 100644 --- a/src/wallet.ts +++ b/src/wallet.ts @@ -276,6 +276,7 @@ WalletMiddlewareOptions): JsonRpcMiddleware { } const params = req.params as [string, string]; + const address = await validateAndNormalizeKeyholder(params[0], req); const message = normalizeTypedMessage(params[1]); validateVerifyingContract(message); @@ -306,6 +307,7 @@ WalletMiddlewareOptions): JsonRpcMiddleware { } const params = req.params as [string, string]; + const address = await validateAndNormalizeKeyholder(params[0], req); const message = normalizeTypedMessage(params[1]); validateVerifyingContract(message); From b1b150b6db0ebc750e5302df3a265051d715f239 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Thu, 18 Jul 2024 21:15:21 +0530 Subject: [PATCH 3/4] fix --- src/utils/normalize.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/normalize.test.ts b/src/utils/normalize.test.ts index 77bb42e0..06cc2091 100644 --- a/src/utils/normalize.test.ts +++ b/src/utils/normalize.test.ts @@ -66,14 +66,14 @@ describe('normalizeTypedMessage', () => { } it('should normalize verifyingContract address in domain', () => { - const msg_mock = { + const msgMock = { ...MESSAGE_DATA_MOCK, domain: { ...MESSAGE_DATA_MOCK.domain, verifyingContract: '0Xae7ab96520de3a18e5e111b5eaab095312d7fe84', }, }; - const normalizedData = parseNormalizerResult(msg_mock); + const normalizedData = parseNormalizerResult(msgMock); expect(normalizedData.domain.verifyingContract).toBe( '0xae7ab96520de3a18e5e111b5eaab095312d7fe84', ); From 294bab06b16d4b586ae234f113a7b2e581b3ace8 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Fri, 19 Jul 2024 05:26:13 +0530 Subject: [PATCH 4/4] fix --- .github/workflows/build-lint-test.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-lint-test.yml b/.github/workflows/build-lint-test.yml index bcab1c3a..e652d44d 100644 --- a/.github/workflows/build-lint-test.yml +++ b/.github/workflows/build-lint-test.yml @@ -32,7 +32,7 @@ jobs: with: node-version: ${{ matrix.node-version }} cache: 'yarn' - - run: yarn --immutable --immutable-cache + - run: yarn --immutable - run: yarn build - name: Require clean working directory shell: bash @@ -57,7 +57,7 @@ jobs: with: node-version: ${{ matrix.node-version }} cache: 'yarn' - - run: yarn --immutable --immutable-cache + - run: yarn --immutable - run: yarn lint - name: Validate RC changelog if: ${{ startsWith(github.head_ref, 'release/') }} @@ -88,7 +88,7 @@ jobs: with: node-version: ${{ matrix.node-version }} cache: 'yarn' - - run: yarn --immutable --immutable-cache + - run: yarn --immutable - run: yarn test - name: Require clean working directory shell: bash