diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index 5910f00bf9b..ac71dfab863 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -492,14 +492,8 @@ "packages/selected-network-controller/tests/SelectedNetworkController.test.ts": { "jest/no-conditional-in-test": 1 }, - "packages/signature-controller/src/SignatureController.test.ts": { - "import-x/order": 1, - "jsdoc/tag-lines": 3 - }, "packages/signature-controller/src/SignatureController.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 4, - "@typescript-eslint/prefer-readonly": 3, - "jsdoc/tag-lines": 8 + "@typescript-eslint/no-unsafe-enum-comparison": 4 }, "packages/signature-controller/src/utils/decoding-api.test.ts": { "import-x/order": 1, @@ -515,13 +509,9 @@ "@typescript-eslint/no-unused-vars": 1, "jsdoc/tag-lines": 2 }, - "packages/signature-controller/src/utils/validation.test.ts": { - "import-x/order": 1 - }, "packages/signature-controller/src/utils/validation.ts": { "@typescript-eslint/no-base-to-string": 1, - "@typescript-eslint/no-unused-vars": 2, - "jsdoc/tag-lines": 4 + "@typescript-eslint/no-unused-vars": 2 }, "packages/user-operation-controller/src/UserOperationController.test.ts": { "jsdoc/tag-lines": 4 diff --git a/packages/signature-controller/package.json b/packages/signature-controller/package.json index 8399abf7519..bcef81ec23c 100644 --- a/packages/signature-controller/package.json +++ b/packages/signature-controller/package.json @@ -56,6 +56,7 @@ "uuid": "^8.3.2" }, "devDependencies": { + "@metamask/accounts-controller": "^26.0.0", "@metamask/approval-controller": "^7.1.3", "@metamask/auto-changelog": "^3.4.4", "@metamask/keyring-controller": "^21.0.0", @@ -70,6 +71,7 @@ "typescript": "~5.2.2" }, "peerDependencies": { + "@metamask/accounts-controller": "^26.0.0", "@metamask/approval-controller": "^7.0.0", "@metamask/keyring-controller": "^21.0.0", "@metamask/logging-controller": "^6.0.0", diff --git a/packages/signature-controller/src/SignatureController.test.ts b/packages/signature-controller/src/SignatureController.test.ts index 3c903a87667..8a5c53b3982 100644 --- a/packages/signature-controller/src/SignatureController.test.ts +++ b/packages/signature-controller/src/SignatureController.test.ts @@ -4,7 +4,6 @@ import { SignTypedDataVersion } from '@metamask/keyring-controller'; import { LogType, SigningStage } from '@metamask/logging-controller'; import { v1 } from 'uuid'; -import { flushPromises } from '../../../tests/helpers'; import type { SignatureControllerMessenger, SignatureControllerOptions, @@ -23,6 +22,8 @@ import { normalizePersonalMessageParams, normalizeTypedMessageParams, } from './utils/normalize'; +import { validateTypedSignatureRequest } from './utils/validation'; +import { flushPromises } from '../../../tests/helpers'; jest.mock('uuid'); jest.mock('./utils/validation'); @@ -89,26 +90,30 @@ const PERMIT_REQUEST_MOCK = { /** * Create a mock messenger instance. + * * @returns The mock messenger instance plus individual mock functions for each action. */ function createMessengerMock() { - const loggingControllerAddMock = jest.fn(); + const accountsControllerGetStateMock = jest.fn(); const approvalControllerAddRequestMock = jest.fn(); const keyringControllerSignPersonalMessageMock = jest.fn(); const keyringControllerSignTypedMessageMock = jest.fn(); + const loggingControllerAddMock = jest.fn(); const networkControllerGetNetworkClientByIdMock = jest.fn(); // eslint-disable-next-line @typescript-eslint/no-explicit-any const callMock = (method: string, ...args: any[]) => { switch (method) { - case 'LoggingController:add': - return loggingControllerAddMock(...args); + case 'AccountsController:getState': + return accountsControllerGetStateMock(...args); case 'ApprovalController:addRequest': return approvalControllerAddRequestMock(...args); case 'KeyringController:signPersonalMessage': return keyringControllerSignPersonalMessageMock(...args); case 'KeyringController:signTypedMessage': return keyringControllerSignTypedMessageMock(...args); + case 'LoggingController:add': + return loggingControllerAddMock(...args); case 'NetworkController:getNetworkClientById': return networkControllerGetNetworkClientByIdMock(...args); default: @@ -123,6 +128,12 @@ function createMessengerMock() { call: callMock, } as unknown as jest.Mocked; + accountsControllerGetStateMock.mockReturnValue({ + internalAccounts: { + accounts: [], + }, + }); + approvalControllerAddRequestMock.mockResolvedValue({}); loggingControllerAddMock.mockResolvedValue({}); @@ -133,6 +144,7 @@ function createMessengerMock() { }); return { + accountsControllerGetStateMock, approvalControllerAddRequestMock, keyringControllerSignPersonalMessageMock, keyringControllerSignTypedMessageMock, @@ -143,6 +155,7 @@ function createMessengerMock() { /** * Create a new instance of the SignatureController. + * * @param options - Optional overrides for the default options. * @returns The controller instance plus individual mock functions for each action. */ @@ -159,6 +172,7 @@ function createController(options?: Partial) { /** * Create a mock error. + * * @returns The mock error instance. */ function createErrorMock(): Error { @@ -177,6 +191,10 @@ describe('SignatureController', () => { normalizeTypedMessageParams, ); + const validateTypedSignatureRequestMock = jest.mocked( + validateTypedSignatureRequest, + ); + const detectSIWEMock = jest.mocked(detectSIWE); const uuidV1Mock = jest.mocked(v1); @@ -1068,6 +1086,56 @@ describe('SignatureController', () => { controller.state.signatureRequests[ID_MOCK].decodingLoading, ).toBe(true); }); + + it('validates the request', async () => { + const { controller } = createController(); + + await controller.newUnsignedTypedMessage( + PARAMS_MOCK, + REQUEST_MOCK, + SignTypedDataVersion.V4, + { parseJsonData: false }, + ); + + expect(validateTypedSignatureRequestMock).toHaveBeenCalledTimes(1); + }); + + it('validates the request using EOA internal accounts', async () => { + const { controller, accountsControllerGetStateMock } = + createController(); + + accountsControllerGetStateMock.mockReturnValue({ + internalAccounts: { + accounts: [ + { + type: 'eip155:eoa', + address: '0x123', + }, + { + type: 'invalid', + address: '0x321', + }, + { + type: 'eip155:eoa', + address: '0xabc', + }, + ], + }, + }); + + await controller.newUnsignedTypedMessage( + PARAMS_MOCK, + REQUEST_MOCK, + SignTypedDataVersion.V4, + { parseJsonData: false }, + ); + + expect(validateTypedSignatureRequestMock).toHaveBeenCalledWith( + expect.objectContaining({ + internalAccounts: ['0x123', '0xabc'], + }), + ); + }); }); }); diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index af4347b6f31..26b80542cef 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -1,3 +1,4 @@ +import type { AccountsControllerGetStateAction } from '@metamask/accounts-controller'; import type { AddApprovalRequest, AcceptResultCallbacks, @@ -89,30 +90,35 @@ export type SignatureControllerState = { /** * Map of personal messages with the unapproved status, keyed by ID. + * * @deprecated - Use `signatureRequests` instead. */ unapprovedPersonalMsgs: Record; /** * Map of typed messages with the unapproved status, keyed by ID. + * * @deprecated - Use `signatureRequests` instead. */ unapprovedTypedMessages: Record; /** * Number of unapproved personal messages. + * * @deprecated - Use `signatureRequests` instead. */ unapprovedPersonalMsgCount: number; /** * Number of unapproved typed messages. + * * @deprecated - Use `signatureRequests` instead. */ unapprovedTypedMessagesCount: number; }; type AllowedActions = + | AccountsControllerGetStateAction | AddApprovalRequest | KeyringControllerSignMessageAction | KeyringControllerSignPersonalMessageAction @@ -189,11 +195,11 @@ export class SignatureController extends BaseController< > { hub: EventEmitter; - #decodingApiUrl?: string; + readonly #decodingApiUrl?: string; - #isDecodeSignatureRequestEnabled?: () => boolean; + readonly #isDecodeSignatureRequestEnabled?: () => boolean; - #trace: TraceCallback; + readonly #trace: TraceCallback; /** * Construct a Sign controller. @@ -230,6 +236,7 @@ export class SignatureController extends BaseController< /** * A getter for the number of 'unapproved' PersonalMessages in this.messages. + * * @deprecated Use `signatureRequests` state instead. * @returns The number of 'unapproved' PersonalMessages in this.messages */ @@ -239,6 +246,7 @@ export class SignatureController extends BaseController< /** * A getter for the number of 'unapproved' TypedMessages in this.messages. + * * @deprecated Use `signatureRequests` state instead. * @returns The number of 'unapproved' TypedMessages in this.messages */ @@ -248,6 +256,7 @@ export class SignatureController extends BaseController< /** * A getter for returning all messages. + * * @deprecated Use `signatureRequests` state instead. * @returns The object containing all messages. */ @@ -346,12 +355,15 @@ export class SignatureController extends BaseController< options: { traceContext?: TraceContext } = {}, ): Promise { const chainId = this.#getChainId(request); + const internalAccounts = this.#getInternalAccounts(); - validateTypedSignatureRequest( - messageParams, - version as SignTypedDataVersion, - chainId, - ); + validateTypedSignatureRequest({ + currentChainId: chainId, + internalAccounts, + messageData: messageParams, + request, + version: version as SignTypedDataVersion, + }); const normalizedMessageParams = normalizeTypedMessageParams( messageParams, @@ -386,6 +398,7 @@ export class SignatureController extends BaseController< /** * Set custom metadata on a signature request. + * * @param signatureRequestId - The ID of the signature request. * @param metadata - The custom metadata to set. */ @@ -938,4 +951,13 @@ export class SignatureController extends BaseController< }), ); } + + #getInternalAccounts(): Hex[] { + const state = this.messagingSystem.call('AccountsController:getState'); + + /* istanbul ignore next */ + return Object.values(state.internalAccounts?.accounts ?? {}) + .filter((account) => account.type === 'eip155:eoa') + .map((account) => account.address as Hex); + } } diff --git a/packages/signature-controller/src/types.ts b/packages/signature-controller/src/types.ts index b4610a96cde..424cf2d0411 100644 --- a/packages/signature-controller/src/types.ts +++ b/packages/signature-controller/src/types.ts @@ -86,18 +86,18 @@ export type MessageParamsPersonal = MessageParams & { siwe?: StateSIWEMessage; }; +/** Typed data used in the signTypedData request. */ +export type MessageParamsTypedData = { + types: Record; + domain: Record; + primaryType: string; + message: Json; +}; + /** Typed message parameters that were requested to be signed. */ export type MessageParamsTyped = MessageParams & { /** Structured data to sign. */ - data: - | Record[] - | string - | { - types: Record; - domain: Record; - primaryType: string; - message: Json; - }; + data: Record[] | string | MessageParamsTypedData; /** Version of the signTypedData request. */ version?: string; }; diff --git a/packages/signature-controller/src/utils/validation.test.ts b/packages/signature-controller/src/utils/validation.test.ts index c02493f40da..9120bbe0ec4 100644 --- a/packages/signature-controller/src/utils/validation.test.ts +++ b/packages/signature-controller/src/utils/validation.test.ts @@ -1,21 +1,29 @@ +import { ORIGIN_METAMASK } from '@metamask/approval-controller'; import { convertHexToDecimal, toHex } from '@metamask/controller-utils'; import { SignTypedDataVersion } from '@metamask/keyring-controller'; +import type { Hex } from '@metamask/utils'; +import { + PRIMARY_TYPE_DELEGATION, + validatePersonalSignatureRequest, + validateTypedSignatureRequest, +} from './validation'; import type { MessageParams, MessageParamsPersonal, MessageParamsTyped, + OriginalRequest, } from '../types'; -import { - validatePersonalSignatureRequest, - validateTypedSignatureRequest, -} from './validation'; const CHAIN_ID_MOCK = '0x1'; +const ORIGIN_MOCK = 'test.com'; +const INTERNAL_ACCOUNT_MOCK = '0x12345678abcd'; const DATA_TYPED_MOCK = '{"types":{"EIP712Domain":[{"name":"name","type":"string"},{"name":"version","type":"string"},{"name":"chainId","type":"uint256"},{"name":"verifyingContract","type":"address"}],"Person":[{"name":"name","type":"string"},{"name":"wallet","type":"address"}],"Mail":[{"name":"from","type":"Person"},{"name":"to","type":"Person"},{"name":"contents","type":"string"}]},"primaryType":"Mail","domain":{"name":"Ether Mail","version":"1","chainId":1,"verifyingContract":"0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC"},"message":{"from":{"name":"Cow","wallet":"0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826"},"to":{"name":"Bob","wallet":"0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB"},"contents":"Hello, Bob!"}}'; +const REQUEST_MOCK = {} as OriginalRequest; + describe('Validation Utils', () => { describe.each([ [ @@ -26,11 +34,13 @@ describe('Validation Utils', () => { [ 'validateTypedSignatureRequest', (params: MessageParams) => - validateTypedSignatureRequest( - params as MessageParamsTyped, - SignTypedDataVersion.V1, - CHAIN_ID_MOCK, - ), + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: params as MessageParamsTyped, + request: REQUEST_MOCK, + version: SignTypedDataVersion.V1, + }), ], ] as const)('%s', (_title, fn) => { it('throws if no from address', () => { @@ -83,39 +93,45 @@ describe('Validation Utils', () => { describe('V1', () => { it('throws if incorrect data', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { data: '0x879a05', from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, - SignTypedDataVersion.V1, - CHAIN_ID_MOCK, - ), + request: REQUEST_MOCK, + version: SignTypedDataVersion.V1, + }), ).toThrow('Invalid message "data":'); }); it('throws if no data', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { from: '0x3244e191f1b4903970224322180f1fbbc415696b', } as MessageParamsTyped, - SignTypedDataVersion.V1, - CHAIN_ID_MOCK, - ), + request: REQUEST_MOCK, + version: SignTypedDataVersion.V1, + }), ).toThrow('Invalid message "data":'); }); it('throws if invalid type data', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { data: [], from: '0x3244e191f1b4903970224322180f1fbbc415696b', } as MessageParamsTyped, - SignTypedDataVersion.V1, - CHAIN_ID_MOCK, - ), + request: REQUEST_MOCK, + version: SignTypedDataVersion.V1, + }), ).toThrow('Expected EIP712 typed data.'); }); }); @@ -125,52 +141,60 @@ describe('Validation Utils', () => { (version) => { it('throws if array data', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { data: [], from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, + request: REQUEST_MOCK, version, - CHAIN_ID_MOCK, - ), + }), ).toThrow('Invalid message "data":'); }); it('throws if no array data', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { from: '0x3244e191f1b4903970224322180f1fbbc415696b', } as MessageParamsTyped, + request: REQUEST_MOCK, version, - CHAIN_ID_MOCK, - ), + }), ).toThrow('Invalid message "data":'); }); it('throws if no JSON valid data', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { data: 'uh oh', from: '0x3244e191f1b4903970224322180f1fbbc415696b', } as MessageParamsTyped, + request: REQUEST_MOCK, version, - CHAIN_ID_MOCK, - ), + }), ).toThrow('Data must be passed as a valid JSON string.'); }); it('throws if current chain ID is not present', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: undefined, + internalAccounts: [], + messageData: { data: DATA_TYPED_MOCK, from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, + request: REQUEST_MOCK, version, - undefined, - ), + }), ).toThrow('Current chainId cannot be null or undefined.'); }); @@ -178,14 +202,16 @@ describe('Validation Utils', () => { const unexpectedChainId = 'unexpected chain id'; expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: unexpectedChainId as never, + internalAccounts: [], + messageData: { data: DATA_TYPED_MOCK.replace(`"chainId":1`, `"chainId":"0x1"`), from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, + request: REQUEST_MOCK, version, - unexpectedChainId as never, - ), + }), ).toThrow( `Cannot sign messages for chainId "${String( convertHexToDecimal(CHAIN_ID_MOCK), @@ -197,21 +223,22 @@ describe('Validation Utils', () => { const chainId = toHex(2); expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: chainId, + internalAccounts: [], + messageData: { data: DATA_TYPED_MOCK, from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, + request: REQUEST_MOCK, version, - chainId, - ), + }), ).toThrow( // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + `Provided chainId "${convertHexToDecimal( CHAIN_ID_MOCK, // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions )}" must match the active chainId "${convertHexToDecimal( chainId, )}"`, @@ -220,42 +247,267 @@ describe('Validation Utils', () => { it('throws if data not in typed message schema', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { data: '{"greetings":"I am Alice"}', from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, + request: REQUEST_MOCK, version, - CHAIN_ID_MOCK, - ), + }), ).toThrow('Data must conform to EIP-712 schema.'); }); it('does not throw if data is correct', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { data: DATA_TYPED_MOCK.replace(`"chainId":1`, `"chainId":"1"`), from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, + request: REQUEST_MOCK, version, - CHAIN_ID_MOCK, - ), + }), ).not.toThrow(); }); it('does not throw if data is correct (object format)', () => { expect(() => - validateTypedSignatureRequest( - { + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [], + messageData: { data: JSON.parse(DATA_TYPED_MOCK), from: '0x3244e191f1b4903970224322180f1fbbc415696b', }, + request: REQUEST_MOCK, version, - CHAIN_ID_MOCK, - ), + }), ).not.toThrow(); }); + + describe('verifying contract', () => { + it('throws if external origin in request and verifying contract is internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + data.domain.verifyingContract = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: { origin: ORIGIN_MOCK } as OriginalRequest, + version, + }), + ).toThrow( + 'External signature requests cannot use internal accounts as the verifying contract.', + ); + }); + + it('throws if external origin in message params and verifying contract is internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + data.domain.verifyingContract = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + origin: ORIGIN_MOCK, + }, + request: REQUEST_MOCK, + version, + }), + ).toThrow( + 'External signature requests cannot use internal accounts as the verifying contract.', + ); + }); + + it('throws if external origin and verifying contract is internal account with different case', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + data.domain.verifyingContract = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [ + '0x1234', + INTERNAL_ACCOUNT_MOCK.toUpperCase() as Hex, + ], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: { origin: ORIGIN_MOCK } as OriginalRequest, + version, + }), + ).toThrow( + 'External signature requests cannot use internal accounts as the verifying contract.', + ); + }); + + it('does not throw if internal origin and verifying contract is internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + data.domain.verifyingContract = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: { origin: ORIGIN_METAMASK } as OriginalRequest, + version, + }), + ).not.toThrow(); + }); + + it('does not throw if no origin and verifying contract is internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + data.domain.verifyingContract = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: REQUEST_MOCK, + version, + }), + ).not.toThrow(); + }); + }); + + describe('delegation', () => { + it('throws if external origin in request and delegation from internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + + data.primaryType = PRIMARY_TYPE_DELEGATION; + data.types.Delegation = [{ name: 'delegator', type: 'address' }]; + data.message.delegator = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: { origin: ORIGIN_MOCK } as OriginalRequest, + version, + }), + ).toThrow( + 'External signature requests cannot sign delegations for internal accounts.', + ); + }); + + it('throws if external origin in message params and delegation from internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + + data.primaryType = PRIMARY_TYPE_DELEGATION; + data.types.Delegation = [{ name: 'delegator', type: 'address' }]; + data.message.delegator = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + origin: ORIGIN_MOCK, + }, + request: REQUEST_MOCK, + version, + }), + ).toThrow( + 'External signature requests cannot sign delegations for internal accounts.', + ); + }); + + it('throws if external origin and delegation from internal account with different case', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + + data.primaryType = PRIMARY_TYPE_DELEGATION; + data.types.Delegation = [{ name: 'delegator', type: 'address' }]; + data.message.delegator = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: [ + '0x1234', + INTERNAL_ACCOUNT_MOCK.toUpperCase() as Hex, + ], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: { origin: ORIGIN_MOCK } as OriginalRequest, + version, + }), + ).toThrow( + 'External signature requests cannot sign delegations for internal accounts.', + ); + }); + + it('does not throw if internal origin and delegation from internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + + data.primaryType = PRIMARY_TYPE_DELEGATION; + data.types.Delegation = [{ name: 'delegator', type: 'address' }]; + data.message.delegator = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: { origin: ORIGIN_METAMASK } as OriginalRequest, + version, + }), + ).not.toThrow(); + }); + + it('does not throw if no origin and delegation from internal account', () => { + const data = JSON.parse(DATA_TYPED_MOCK); + + data.primaryType = PRIMARY_TYPE_DELEGATION; + data.types.Delegation = [{ name: 'delegator', type: 'address' }]; + data.message.delegator = INTERNAL_ACCOUNT_MOCK; + + expect(() => + validateTypedSignatureRequest({ + currentChainId: CHAIN_ID_MOCK, + internalAccounts: ['0x1234', INTERNAL_ACCOUNT_MOCK], + messageData: { + data, + from: '0x3244e191f1b4903970224322180f1fbbc415696b', + }, + request: REQUEST_MOCK, + version, + }), + ).not.toThrow(); + }); + }); }, ); }); diff --git a/packages/signature-controller/src/utils/validation.ts b/packages/signature-controller/src/utils/validation.ts index cf4ed87d900..23ea5351104 100644 --- a/packages/signature-controller/src/utils/validation.ts +++ b/packages/signature-controller/src/utils/validation.ts @@ -1,16 +1,27 @@ +import { ORIGIN_METAMASK } from '@metamask/approval-controller'; import { isValidHexAddress } from '@metamask/controller-utils'; import { TYPED_MESSAGE_SCHEMA, typedSignatureHash, } from '@metamask/eth-sig-util'; import { SignTypedDataVersion } from '@metamask/keyring-controller'; +import type { Json } from '@metamask/utils'; import { type Hex } from '@metamask/utils'; import { validate } from 'jsonschema'; -import type { MessageParamsPersonal, MessageParamsTyped } from '../types'; +import type { + MessageParamsPersonal, + MessageParamsTyped, + MessageParamsTypedData, + OriginalRequest, +} from '../types'; + +export const PRIMARY_TYPE_DELEGATION = 'Delegation'; +export const DELEGATOR_FIELD = 'delegator'; /** * Validate a personal signature request. + * * @param messageData - The message data to validate. */ export function validatePersonalSignatureRequest( @@ -27,26 +38,44 @@ export function validatePersonalSignatureRequest( /** * Validate a typed signature request. - * @param messageData - The message data to validate. - * @param version - The version of the typed signature request. - * @param currentChainId - The current chain ID. + * + * @param options - Options bag. + * @param options.currentChainId - The current chain ID. + * @param options.internalAccounts - The addresses of all internal accounts. + * @param options.messageData - The message data to validate. + * @param options.request - The original request. + * @param options.version - The version of the typed signature request. */ -export function validateTypedSignatureRequest( - messageData: MessageParamsTyped, - version: SignTypedDataVersion, - currentChainId: Hex | undefined, -) { +export function validateTypedSignatureRequest({ + currentChainId, + internalAccounts, + messageData, + request, + version, +}: { + currentChainId: Hex | undefined; + internalAccounts: Hex[]; + messageData: MessageParamsTyped; + request: OriginalRequest; + version: SignTypedDataVersion; +}) { validateAddress(messageData.from, 'from'); if (version === SignTypedDataVersion.V1) { validateTypedSignatureRequestV1(messageData); } else { - validateTypedSignatureRequestV3V4(messageData, currentChainId); + validateTypedSignatureRequestV3V4({ + currentChainId, + internalAccounts, + messageData, + request, + }); } } /** * Validate a V1 typed signature request. + * * @param messageData - The message data to validate. */ function validateTypedSignatureRequestV1(messageData: MessageParamsTyped) { @@ -71,13 +100,23 @@ function validateTypedSignatureRequestV1(messageData: MessageParamsTyped) { /** * Validate a V3 or V4 typed signature request. * - * @param messageData - The message data to validate. - * @param currentChainId - The current chain ID. + * @param options - Options bag. + * @param options.currentChainId - The current chain ID. + * @param options.internalAccounts - The addresses of all internal accounts. + * @param options.messageData - The message data to validate. + * @param options.request - The original request. */ -function validateTypedSignatureRequestV3V4( - messageData: MessageParamsTyped, - currentChainId: Hex | undefined, -) { +function validateTypedSignatureRequestV3V4({ + currentChainId, + internalAccounts, + messageData, + request, +}: { + currentChainId: Hex | undefined; + internalAccounts: Hex[]; + messageData: MessageParamsTyped; + request: OriginalRequest; +}) { if ( !messageData.data || Array.isArray(messageData.data) || @@ -134,10 +173,25 @@ function validateTypedSignatureRequestV3V4( ); } } + + const origin = request?.origin ?? messageData?.origin; + + validateVerifyingContract({ + data, + internalAccounts, + origin, + }); + + validateDelegation({ + data, + internalAccounts, + origin, + }); } /** * Validate an Ethereum address. + * * @param address - The address to validate. * @param propertyName - The name of the property source to use in the error message. */ @@ -148,3 +202,77 @@ function validateAddress(address: string, propertyName: string) { ); } } + +/** + * Validate the verifying contract from a typed signature request. + * + * @param options - Options bag. + * @param options.data - The typed data to validate. + * @param options.internalAccounts - The internal accounts. + * @param options.origin - The origin of the request. + */ +function validateVerifyingContract({ + data, + internalAccounts, + origin, +}: { + data: MessageParamsTypedData; + internalAccounts: Hex[]; + origin: string | undefined; +}) { + const verifyingContract = data?.domain?.verifyingContract as Hex; + const isExternal = origin && origin !== ORIGIN_METAMASK; + + if ( + isExternal && + internalAccounts.some( + (internalAccount) => + internalAccount.toLowerCase() === verifyingContract.toLowerCase(), + ) + ) { + throw new Error( + `External signature requests cannot use internal accounts as the verifying contract.`, + ); + } +} + +/** + * Validate a delegation signature request. + * + * @param options - Options bag. + * @param options.data - The typed data to validate. + * @param options.internalAccounts - The internal accounts. + * @param options.origin - The origin of the request. + */ +function validateDelegation({ + data, + internalAccounts, + origin, +}: { + data: MessageParamsTypedData; + internalAccounts: Hex[]; + origin: string | undefined; +}) { + const { primaryType } = data; + + if (primaryType !== PRIMARY_TYPE_DELEGATION) { + return; + } + + const isExternal = origin && origin !== ORIGIN_METAMASK; + const delegator = (data.message as Record)?.[ + DELEGATOR_FIELD + ] as Hex; + + if ( + isExternal && + internalAccounts.some( + (internalAccount) => + internalAccount.toLowerCase() === delegator?.toLowerCase(), + ) + ) { + throw new Error( + `External signature requests cannot sign delegations for internal accounts.`, + ); + } +} diff --git a/packages/signature-controller/tsconfig.build.json b/packages/signature-controller/tsconfig.build.json index c7831754a6e..6a43384346f 100644 --- a/packages/signature-controller/tsconfig.build.json +++ b/packages/signature-controller/tsconfig.build.json @@ -6,6 +6,9 @@ "rootDir": "./src" }, "references": [ + { + "path": "../accounts-controller/tsconfig.build.json" + }, { "path": "../approval-controller/tsconfig.build.json" }, diff --git a/packages/signature-controller/tsconfig.json b/packages/signature-controller/tsconfig.json index 11bf1c18982..f9d020df047 100644 --- a/packages/signature-controller/tsconfig.json +++ b/packages/signature-controller/tsconfig.json @@ -4,6 +4,9 @@ "baseUrl": "./" }, "references": [ + { + "path": "../accounts-controller" + }, { "path": "../approval-controller" }, diff --git a/yarn.lock b/yarn.lock index ede725f5133..63f0bf7ceb3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4057,6 +4057,7 @@ __metadata: version: 0.0.0-use.local resolution: "@metamask/signature-controller@workspace:packages/signature-controller" dependencies: + "@metamask/accounts-controller": "npm:^26.0.0" "@metamask/approval-controller": "npm:^7.1.3" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^8.0.0" @@ -4077,6 +4078,7 @@ __metadata: typescript: "npm:~5.2.2" uuid: "npm:^8.3.2" peerDependencies: + "@metamask/accounts-controller": ^26.0.0 "@metamask/approval-controller": ^7.0.0 "@metamask/keyring-controller": ^21.0.0 "@metamask/logging-controller": ^6.0.0