diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 801cb22f6f9..2d675e408e3 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Throw if `addTransactionBatch` is called with any nested transaction with `to` matching internal account ([#5369](https://github.com/MetaMask/core/pull/5369)) - **BREAKING:** Support atomic batch transactions ([#5306](https://github.com/MetaMask/core/pull/5306)) - Require `AccountsController:getState` action permission in messenger. - Require `RemoteFeatureFlagController:getState` action permission in messenger. diff --git a/packages/transaction-controller/package.json b/packages/transaction-controller/package.json index 7dd743fda9f..d5e6a1df94d 100644 --- a/packages/transaction-controller/package.json +++ b/packages/transaction-controller/package.json @@ -58,7 +58,6 @@ "@metamask/eth-query": "^4.0.0", "@metamask/metamask-eth-abis": "^3.1.1", "@metamask/nonce-tracker": "^6.0.0", - "@metamask/remote-feature-flag-controller": "^1.5.0", "@metamask/rpc-errors": "^7.0.2", "@metamask/utils": "^11.2.0", "async-mutex": "^0.5.0", @@ -78,6 +77,7 @@ "@metamask/ethjs-provider-http": "^0.3.0", "@metamask/gas-fee-controller": "^22.0.3", "@metamask/network-controller": "^22.2.1", + "@metamask/remote-feature-flag-controller": "^1.5.0", "@types/bn.js": "^5.1.5", "@types/jest": "^27.4.1", "@types/node": "^16.18.54", @@ -98,7 +98,7 @@ "@metamask/eth-block-tracker": ">=9", "@metamask/gas-fee-controller": "^22.0.0", "@metamask/network-controller": "^22.0.0", - "@metamask/remote-feature-flag-controller": "^1.3.0" + "@metamask/remote-feature-flag-controller": "^1.5.0" }, "engines": { "node": "^18.18 || >=20" diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index b7bc38a6849..3e7b6dd956c 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -990,6 +990,7 @@ export class TransactionController extends BaseController< addTransaction: this.addTransaction.bind(this), getChainId: this.#getChainId.bind(this), getEthQuery: (networkClientId) => this.#getEthQuery({ networkClientId }), + getInternalAccounts: this.#getInternalAccounts.bind(this), messenger: this.messagingSystem, request, }); @@ -3805,12 +3806,12 @@ export class TransactionController extends BaseController< return this.messagingSystem.call('AccountsController:getSelectedAccount'); } - #getInternalAccounts(): string[] { + #getInternalAccounts(): Hex[] { const state = this.messagingSystem.call('AccountsController:getState'); return Object.values(state.internalAccounts?.accounts ?? {}) .filter((account) => account.type === 'eip155:eoa') - .map((account) => account.address); + .map((account) => account.address as Hex); } #updateSubmitHistory(transactionMeta: TransactionMeta, hash: string): void { diff --git a/packages/transaction-controller/src/utils/batch.test.ts b/packages/transaction-controller/src/utils/batch.test.ts index 7e89af20770..13b0ff676ac 100644 --- a/packages/transaction-controller/src/utils/batch.test.ts +++ b/packages/transaction-controller/src/utils/batch.test.ts @@ -10,6 +10,7 @@ import { getEIP7702SupportedChains, getEIP7702UpgradeContractAddress, } from './feature-flags'; +import { validateBatchRequest } from './validation'; import { TransactionEnvelopeType, type TransactionControllerMessenger, @@ -19,6 +20,11 @@ import { jest.mock('./eip7702'); jest.mock('./feature-flags'); +jest.mock('./validation', () => ({ + ...jest.requireActual('./validation'), + validateBatchRequest: jest.fn(), +})); + type AddBatchTransactionOptions = Parameters[0]; const CHAIN_ID_MOCK = '0x123'; @@ -32,6 +38,7 @@ const MESSENGER_MOCK = {} as TransactionControllerMessenger; const NETWORK_CLIENT_ID_MOCK = 'testNetworkClientId'; const BATCH_ID_MOCK = 'testBatchId'; const GET_ETH_QUERY_MOCK = jest.fn(); +const GET_INTERNAL_ACCOUNTS_MOCK = jest.fn().mockReturnValue([]); const TRANSACTION_META_MOCK = { id: BATCH_ID_MOCK, @@ -40,6 +47,7 @@ const TRANSACTION_META_MOCK = { describe('Batch Utils', () => { const doesChainSupportEIP7702Mock = jest.mocked(doesChainSupportEIP7702); const getEIP7702SupportedChainsMock = jest.mocked(getEIP7702SupportedChains); + const validateBatchRequestMock = jest.mocked(validateBatchRequest); const isAccountUpgradedToEIP7702Mock = jest.mocked( isAccountUpgradedToEIP7702, @@ -73,6 +81,7 @@ describe('Batch Utils', () => { addTransaction: addTransactionMock, getChainId: getChainIdMock, getEthQuery: GET_ETH_QUERY_MOCK, + getInternalAccounts: GET_INTERNAL_ACCOUNTS_MOCK, messenger: MESSENGER_MOCK, request: { from: FROM_MOCK, @@ -251,6 +260,16 @@ describe('Batch Utils', () => { rpcErrors.internal('Upgrade contract address not found'), ); }); + + it('validates request', async () => { + validateBatchRequestMock.mockImplementationOnce(() => { + throw new Error('Validation Error'); + }); + + await expect(addTransactionBatch(request)).rejects.toThrow( + 'Validation Error', + ); + }); }); describe('isAtomicBatchSupported', () => { diff --git a/packages/transaction-controller/src/utils/batch.ts b/packages/transaction-controller/src/utils/batch.ts index 26655758e04..1b6d1870145 100644 --- a/packages/transaction-controller/src/utils/batch.ts +++ b/packages/transaction-controller/src/utils/batch.ts @@ -12,6 +12,7 @@ import { getEIP7702SupportedChains, getEIP7702UpgradeContractAddress, } from './feature-flags'; +import { validateBatchRequest } from './validation'; import type { TransactionController, TransactionControllerMessenger } from '..'; import { projectLogger } from '../logger'; import { @@ -26,6 +27,7 @@ type AddTransactionBatchRequest = { addTransaction: TransactionController['addTransaction']; getChainId: (networkClientId: string) => Hex; getEthQuery: (networkClientId: string) => EthQuery; + getInternalAccounts: () => Hex[]; messenger: TransactionControllerMessenger; request: TransactionBatchRequest; }; @@ -50,10 +52,16 @@ export async function addTransactionBatch( const { addTransaction, getChainId, + getInternalAccounts, messenger, request: userRequest, } = request; + validateBatchRequest({ + internalAccounts: getInternalAccounts(), + request: userRequest, + }); + const { from, networkClientId, requireApproval, transactions } = userRequest; log('Adding', userRequest); diff --git a/packages/transaction-controller/src/utils/validation.test.ts b/packages/transaction-controller/src/utils/validation.test.ts index 80a50ad4ea1..06593d5e8bd 100644 --- a/packages/transaction-controller/src/utils/validation.test.ts +++ b/packages/transaction-controller/src/utils/validation.test.ts @@ -2,6 +2,7 @@ import { ORIGIN_METAMASK } from '@metamask/controller-utils'; import { rpcErrors } from '@metamask/rpc-errors'; import { + validateBatchRequest, validateParamTo, validateTransactionOrigin, validateTxParams, @@ -11,6 +12,7 @@ import type { TransactionParams } from '../types'; const FROM_MOCK = '0x1678a085c290ebd122dc42cba69373b5953b831d'; const TO_MOCK = '0xfbb5595c18ca76bab52d66188e4ca50c7d95f77a'; +const ORIGIN_MOCK = 'test-origin'; describe('validation', () => { describe('validateTxParams', () => { @@ -617,7 +619,7 @@ describe('validation', () => { await expect( validateTransactionOrigin({ from: FROM_MOCK, - origin: 'test-origin', + origin: ORIGIN_MOCK, permittedAddresses: ['0x123', '0x456'], selectedAddress: '0x123', txParams: {} as TransactionParams, @@ -633,7 +635,7 @@ describe('validation', () => { expect( await validateTransactionOrigin({ from: FROM_MOCK, - origin: 'test-origin', + origin: ORIGIN_MOCK, permittedAddresses: ['0x123', FROM_MOCK], selectedAddress: '0x123', txParams: {} as TransactionParams, @@ -645,7 +647,7 @@ describe('validation', () => { await expect( validateTransactionOrigin({ from: FROM_MOCK, - origin: 'test-origin', + origin: ORIGIN_MOCK, permittedAddresses: [FROM_MOCK], selectedAddress: '0x123', txParams: { @@ -663,7 +665,7 @@ describe('validation', () => { await expect( validateTransactionOrigin({ from: FROM_MOCK, - origin: 'test-origin', + origin: ORIGIN_MOCK, permittedAddresses: [FROM_MOCK], selectedAddress: '0x123', txParams: { @@ -683,7 +685,7 @@ describe('validation', () => { validateTransactionOrigin({ from: FROM_MOCK, internalAccounts: [TO_MOCK], - origin: 'test-origin', + origin: ORIGIN_MOCK, selectedAddress: '0x123', txParams: { to: TO_MOCK, @@ -701,7 +703,7 @@ describe('validation', () => { await validateTransactionOrigin({ from: FROM_MOCK, internalAccounts: [TO_MOCK], - origin: 'test-origin', + origin: ORIGIN_MOCK, selectedAddress: '0x123', txParams: { to: TO_MOCK, @@ -725,4 +727,84 @@ describe('validation', () => { ); }); }); + + describe('validateBatchRequest', () => { + it('throws if external origin and any transaction target is internal account', () => { + expect(() => + validateBatchRequest({ + internalAccounts: ['0x123', TO_MOCK], + request: { + from: FROM_MOCK, + networkClientId: 'testNetworkClientId', + origin: ORIGIN_MOCK, + transactions: [ + { + params: { + to: '0xabc', + }, + }, + { + params: { + to: TO_MOCK, + }, + }, + ], + }, + }), + ).toThrow( + rpcErrors.invalidParams( + 'External transactions to internal accounts are not supported', + ), + ); + }); + + it('does not throw if no origin and any transaction target is internal account', () => { + expect(() => + validateBatchRequest({ + internalAccounts: ['0x123', TO_MOCK], + request: { + from: FROM_MOCK, + networkClientId: 'testNetworkClientId', + transactions: [ + { + params: { + to: '0xabc', + }, + }, + { + params: { + to: TO_MOCK, + }, + }, + ], + }, + }), + ).not.toThrow(); + }); + + it('does not throw if internal origin and any transaction target is internal account', () => { + expect(() => + validateBatchRequest({ + internalAccounts: ['0x123', TO_MOCK], + request: { + from: FROM_MOCK, + networkClientId: 'testNetworkClientId', + origin: ORIGIN_METAMASK, + transactions: [ + { + params: { + to: '0xabc', + }, + }, + { + params: { + to: TO_MOCK, + }, + }, + ], + }, + }), + ).not.toThrow(); + }); + }); }); diff --git a/packages/transaction-controller/src/utils/validation.ts b/packages/transaction-controller/src/utils/validation.ts index cab442cd7b4..a490c1f8be6 100644 --- a/packages/transaction-controller/src/utils/validation.ts +++ b/packages/transaction-controller/src/utils/validation.ts @@ -5,7 +5,7 @@ import { providerErrors, rpcErrors } from '@metamask/rpc-errors'; import { isStrictHexString, remove0x } from '@metamask/utils'; import { isEIP1559Transaction } from './utils'; -import type { Authorization } from '../types'; +import type { Authorization, TransactionBatchRequest } from '../types'; import { TransactionEnvelopeType, TransactionType, @@ -247,6 +247,43 @@ export function validateParamTo(to?: string) { } } +/** + * Validates a transaction batch request. + * + * @param options - Options bag. + * @param options.internalAccounts - The internal accounts added to the wallet. + * @param options.request - The batch request object. + */ +export function validateBatchRequest({ + internalAccounts, + request, +}: { + internalAccounts: string[]; + request: TransactionBatchRequest; +}) { + const { origin } = request; + const isExternal = origin && origin !== ORIGIN_METAMASK; + + const transactionTargetsNormalized = request.transactions.map((tx) => + tx.params.to?.toLowerCase(), + ); + + const internalAccountsNormalized = internalAccounts.map((account) => + account.toLowerCase(), + ); + + if ( + isExternal && + transactionTargetsNormalized.some((target) => + internalAccountsNormalized.includes(target as string), + ) + ) { + throw rpcErrors.invalidParams( + 'External transactions to internal accounts are not supported', + ); + } +} + /** * Validates input data for transactions. * diff --git a/yarn.lock b/yarn.lock index c43b7c95669..b23bb103dec 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4264,7 +4264,7 @@ __metadata: "@metamask/eth-block-tracker": ">=9" "@metamask/gas-fee-controller": ^22.0.0 "@metamask/network-controller": ^22.0.0 - "@metamask/remote-feature-flag-controller": ^1.3.0 + "@metamask/remote-feature-flag-controller": ^1.5.0 languageName: unknown linkType: soft