Changes in typed signature validation and normalization (#318)

MetaMask · Jul 23, 2024 · a055a59 · a055a59
1 parent 725b8c8
commit a055a59
Show file tree

Hide file tree

Showing 4 changed files with 167 additions and 63 deletions.
diff --git 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 msgMock = {
       ...MESSAGE_DATA_MOCK,
       domain: {
         ...MESSAGE_DATA_MOCK.domain,
-        verifyingContract: '0o123',
+        verifyingContract: '0Xae7ab96520de3a18e5e111b5eaab095312d7fe84',
       },
     };
-
-    const normalizedData = parseNormalizerResult(messageDataWithOctalAddress);
-
+    const normalizedData = parseNormalizerResult(msgMock);
     expect(normalizedData.domain.verifyingContract).toBe(
-      expectedNormalizedOctalAddress,
+      '0xae7ab96520de3a18e5e111b5eaab095312d7fe84',
     );
   });
 

diff --git 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
@@ -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
@@ -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 = {
@@ -278,6 +279,7 @@ WalletMiddlewareOptions): JsonRpcMiddleware<any, Block> {
 
     const address = await validateAndNormalizeKeyholder(params[0], req);
     const message = normalizeTypedMessage(params[1]);
+    validateVerifyingContract(message);
     const version = 'V3';
     const msgParams: TypedMessageParams = {
       data: message,
@@ -308,6 +310,7 @@ WalletMiddlewareOptions): JsonRpcMiddleware<any, Block> {
 
     const address = await validateAndNormalizeKeyholder(params[0], req);
     const message = normalizeTypedMessage(params[1]);
+    validateVerifyingContract(message);
     const version = 'V4';
     const msgParams: TypedMessageParams = {
       data: message,
@@ -490,6 +493,20 @@ WalletMiddlewareOptions): JsonRpcMiddleware<any, Block> {
   }
 }
 
+/**
+ * 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;