feat: prevent external transactions to internal accounts if data included (#5418)

## Explanation

Update validation preventing external transactions to internal accounts
to only throw if `data` is included in the parameters.

## References

## Changelog

See `CHANGELOG.md`.

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

Author: matthewwalsh0

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

packages/transaction-controller/src/TransactionController.ts

@@ -1096,6 +1096,7 @@ export class TransactionController extends BaseController<
     const internalAccounts = this.#getInternalAccounts();
 
     await validateTransactionOrigin({
+      data: txParams.data,
       from: txParams.from,
       internalAccounts,
       origin,

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,33 @@ describe('validation', () => {
         }),
       ).rejects.toThrow(
         rpcErrors.invalidParams(
-          'External transactions to internal accounts are not supported',
+          'External transactions to internal accounts cannot include data',
         ),
       );
     });
 
+    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(
         await validateTransactionOrigin({
@@ -752,9 +776,7 @@ describe('validation', () => {
           },
         }),
       ).toThrow(
-        rpcErrors.invalidParams(
-          'External transactions to internal accounts are not supported',
-        ),
+        rpcErrors.invalidParams('Calls to internal accounts are not supported'),
       );
     });
 

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;
@@ -82,15 +85,18 @@ export async function validateTransactionOrigin({
     );
   }
 
+  const hasData = Boolean(data && data !== '0x');
+
   if (
     isExternal &&
+    hasData &&
     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 +285,7 @@ export function validateBatchRequest({
     )
   ) {
     throw rpcErrors.invalidParams(
-      'External transactions to internal accounts are not supported',
+      'Calls to internal accounts are not supported',
     );
   }
 }