From af470a9a429ed5c291eb9d5423c089aa334c795b Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Mon, 3 Mar 2025 11:01:44 +0000 Subject: [PATCH 1/3] Prevent external transactions to internal accounts if data --- .../src/TransactionController.ts | 1 + .../src/utils/validation.test.ts | 25 +++++++++++++++---- .../src/utils/validation.ts | 8 ++++-- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 436e20a1ccb..c3759d2fcd3 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -1093,6 +1093,7 @@ export class TransactionController extends BaseController< const internalAccounts = this.#getInternalAccounts(); await validateTransactionOrigin({ + data: txParams.data, from: txParams.from, internalAccounts, origin, diff --git a/packages/transaction-controller/src/utils/validation.test.ts b/packages/transaction-controller/src/utils/validation.test.ts index 06593d5e8bd..29cd63653a6 100644 --- a/packages/transaction-controller/src/utils/validation.test.ts +++ b/packages/transaction-controller/src/utils/validation.test.ts @@ -10,6 +10,7 @@ import { import { TransactionEnvelopeType, TransactionType } from '../types'; import type { TransactionParams } from '../types'; +const DATA_MOCK = '0x12345678'; const FROM_MOCK = '0x1678a085c290ebd122dc42cba69373b5953b831d'; const TO_MOCK = '0xfbb5595c18ca76bab52d66188e4ca50c7d95f77a'; const ORIGIN_MOCK = 'test-origin'; @@ -680,9 +681,10 @@ describe('validation', () => { ); }); - it('throws if external and to is internal account', async () => { + it('throws if external and to is internal account and data', async () => { await expect( validateTransactionOrigin({ + data: DATA_MOCK, from: FROM_MOCK, internalAccounts: [TO_MOCK], origin: ORIGIN_MOCK, @@ -693,11 +695,26 @@ describe('validation', () => { }), ).rejects.toThrow( rpcErrors.invalidParams( - 'External transactions to internal accounts are not supported', + 'External transactions to internal accounts cannot include data', ), ); }); + it('does not throw if external and to is internal account but no data', async () => { + expect( + await validateTransactionOrigin({ + data: '', + from: FROM_MOCK, + internalAccounts: [TO_MOCK], + origin: ORIGIN_MOCK, + selectedAddress: '0x123', + txParams: { + to: TO_MOCK, + } as TransactionParams, + }), + ).toBeUndefined(); + }); + it('does not throw if external and to is internal account but type is batch', async () => { expect( await validateTransactionOrigin({ @@ -752,9 +769,7 @@ describe('validation', () => { }, }), ).toThrow( - rpcErrors.invalidParams( - 'External transactions to internal accounts are not supported', - ), + rpcErrors.invalidParams('Calls to internal accounts are not supported'), ); }); diff --git a/packages/transaction-controller/src/utils/validation.ts b/packages/transaction-controller/src/utils/validation.ts index a490c1f8be6..7f79d15c1b0 100644 --- a/packages/transaction-controller/src/utils/validation.ts +++ b/packages/transaction-controller/src/utils/validation.ts @@ -28,6 +28,7 @@ type GasFieldsToValidate = * Validates whether a transaction initiated by a specific 'from' address is permitted by the origin. * * @param options - Options bag. + * @param options.data - The data included in the transaction. * @param options.from - The address from which the transaction is initiated. * @param options.internalAccounts - The internal accounts added to the wallet. * @param options.origin - The origin or source of the transaction. @@ -38,6 +39,7 @@ type GasFieldsToValidate = * @throws Throws an error if the transaction is not permitted. */ export async function validateTransactionOrigin({ + data, from, internalAccounts, origin, @@ -46,6 +48,7 @@ export async function validateTransactionOrigin({ txParams, type, }: { + data?: string; from: string; internalAccounts?: string[]; origin?: string; @@ -84,13 +87,14 @@ export async function validateTransactionOrigin({ if ( isExternal && + data && internalAccounts?.some( (account) => account.toLowerCase() === to?.toLowerCase(), ) && type !== TransactionType.batch ) { throw rpcErrors.invalidParams( - 'External transactions to internal accounts are not supported', + 'External transactions to internal accounts cannot include data', ); } } @@ -279,7 +283,7 @@ export function validateBatchRequest({ ) ) { throw rpcErrors.invalidParams( - 'External transactions to internal accounts are not supported', + 'Calls to internal accounts are not supported', ); } } From ab33029584110ce17b0186dc4322ac5c16520102 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Fri, 7 Mar 2025 14:15:08 +0000 Subject: [PATCH 2/3] Handle 0x data --- .../src/utils/validation.test.ts | 35 +++++++++++-------- .../src/utils/validation.ts | 4 ++- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/transaction-controller/src/utils/validation.test.ts b/packages/transaction-controller/src/utils/validation.test.ts index 29cd63653a6..624de7ee8c9 100644 --- a/packages/transaction-controller/src/utils/validation.test.ts +++ b/packages/transaction-controller/src/utils/validation.test.ts @@ -700,20 +700,27 @@ describe('validation', () => { ); }); - it('does not throw if external and to is internal account but no data', async () => { - expect( - await validateTransactionOrigin({ - data: '', - from: FROM_MOCK, - internalAccounts: [TO_MOCK], - origin: ORIGIN_MOCK, - selectedAddress: '0x123', - txParams: { - to: TO_MOCK, - } as TransactionParams, - }), - ).toBeUndefined(); - }); + it.each([ + ['undefined', undefined], + ['empty', ''], + ['empty hex', '0x'], + ])( + 'does not throw if external and to is internal account but data is %s', + async (_title, data) => { + expect( + await validateTransactionOrigin({ + data, + from: FROM_MOCK, + internalAccounts: [TO_MOCK], + origin: ORIGIN_MOCK, + selectedAddress: '0x123', + txParams: { + to: TO_MOCK, + } as TransactionParams, + }), + ).toBeUndefined(); + }, + ); it('does not throw if external and to is internal account but type is batch', async () => { expect( diff --git a/packages/transaction-controller/src/utils/validation.ts b/packages/transaction-controller/src/utils/validation.ts index 7f79d15c1b0..829c3acfdf6 100644 --- a/packages/transaction-controller/src/utils/validation.ts +++ b/packages/transaction-controller/src/utils/validation.ts @@ -85,9 +85,11 @@ export async function validateTransactionOrigin({ ); } + const hasData = Boolean(data && data !== '0x'); + if ( isExternal && - data && + hasData && internalAccounts?.some( (account) => account.toLowerCase() === to?.toLowerCase(), ) && From 22387993ac274407ad39745d39b5f4f12b9557ab Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Fri, 7 Mar 2025 14:17:31 +0000 Subject: [PATCH 3/3] Update changelog --- packages/transaction-controller/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 171c33c0a7d..2e48f47ca0e 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Prevent external transactions to internal accounts if `data` included ([#5418](https://github.com/MetaMask/core/pull/5418)) + ## [48.0.0] ### Changed