feat: validate nested transactions are not to internal accounts

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.

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"

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 {

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<typeof addTransactionBatch>[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', () => {

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);

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();
+    });
+  });
 });

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.
  *

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