Skip to content

Commit 1b35e86

Browse files
matthewwalsh0matthewwalsh0
authored
feat: validate nested transactions are not to internal accounts (#5369)
## Explanation Throw if `addTransactionBatch` is called with any nested transaction with a `to` matching an internal account. Also fix `@metamask/remote-feature-flag-controller` dependency. ## 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
1 parent bc921d4 commit 1b35e86

File tree

8 files changed

+160
-12
lines changed

8 files changed

+160
-12
lines changed

‎packages/transaction-controller/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2424

2525
### Changed
2626

27+
- Throw if `addTransactionBatch` is called with any nested transaction with `to` matching internal account ([#5369](https://github.com/MetaMask/core/pull/5369))
2728
- **BREAKING:** Support atomic batch transactions ([#5306](https://github.com/MetaMask/core/pull/5306))
2829
- Require `AccountsController:getState` action permission in messenger.
2930
- Require `RemoteFeatureFlagController:getState` action permission in messenger.

‎packages/transaction-controller/package.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@
5858
"@metamask/eth-query": "^4.0.0",
5959
"@metamask/metamask-eth-abis": "^3.1.1",
6060
"@metamask/nonce-tracker": "^6.0.0",
61-
"@metamask/remote-feature-flag-controller": "^1.5.0",
6261
"@metamask/rpc-errors": "^7.0.2",
6362
"@metamask/utils": "^11.2.0",
6463
"async-mutex": "^0.5.0",
@@ -78,6 +77,7 @@
7877
"@metamask/ethjs-provider-http": "^0.3.0",
7978
"@metamask/gas-fee-controller": "^22.0.3",
8079
"@metamask/network-controller": "^22.2.1",
80+
"@metamask/remote-feature-flag-controller": "^1.5.0",
8181
"@types/bn.js": "^5.1.5",
8282
"@types/jest": "^27.4.1",
8383
"@types/node": "^16.18.54",
@@ -98,7 +98,7 @@
9898
"@metamask/eth-block-tracker": ">=9",
9999
"@metamask/gas-fee-controller": "^22.0.0",
100100
"@metamask/network-controller": "^22.0.0",
101-
"@metamask/remote-feature-flag-controller": "^1.3.0"
101+
"@metamask/remote-feature-flag-controller": "^1.5.0"
102102
},
103103
"engines": {
104104
"node": "^18.18 || >=20"

‎packages/transaction-controller/src/TransactionController.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -990,6 +990,7 @@ export class TransactionController extends BaseController<
990990
addTransaction: this.addTransaction.bind(this),
991991
getChainId: this.#getChainId.bind(this),
992992
getEthQuery: (networkClientId) => this.#getEthQuery({ networkClientId }),
993+
getInternalAccounts: this.#getInternalAccounts.bind(this),
993994
messenger: this.messagingSystem,
994995
request,
995996
});
@@ -3805,12 +3806,12 @@ export class TransactionController extends BaseController<
38053806
return this.messagingSystem.call('AccountsController:getSelectedAccount');
38063807
}
38073808

3808-
#getInternalAccounts(): string[] {
3809+
#getInternalAccounts(): Hex[] {
38093810
const state = this.messagingSystem.call('AccountsController:getState');
38103811

38113812
return Object.values(state.internalAccounts?.accounts ?? {})
38123813
.filter((account) => account.type === 'eip155:eoa')
3813-
.map((account) => account.address);
3814+
.map((account) => account.address as Hex);
38143815
}
38153816

38163817
#updateSubmitHistory(transactionMeta: TransactionMeta, hash: string): void {

‎packages/transaction-controller/src/utils/batch.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
getEIP7702SupportedChains,
1111
getEIP7702UpgradeContractAddress,
1212
} from './feature-flags';
13+
import { validateBatchRequest } from './validation';
1314
import {
1415
TransactionEnvelopeType,
1516
type TransactionControllerMessenger,
@@ -19,6 +20,11 @@ import {
1920
jest.mock('./eip7702');
2021
jest.mock('./feature-flags');
2122

23+
jest.mock('./validation', () => ({
24+
...jest.requireActual('./validation'),
25+
validateBatchRequest: jest.fn(),
26+
}));
27+
2228
type AddBatchTransactionOptions = Parameters<typeof addTransactionBatch>[0];
2329

2430
const CHAIN_ID_MOCK = '0x123';
@@ -32,6 +38,7 @@ const MESSENGER_MOCK = {} as TransactionControllerMessenger;
3238
const NETWORK_CLIENT_ID_MOCK = 'testNetworkClientId';
3339
const BATCH_ID_MOCK = 'testBatchId';
3440
const GET_ETH_QUERY_MOCK = jest.fn();
41+
const GET_INTERNAL_ACCOUNTS_MOCK = jest.fn().mockReturnValue([]);
3542

3643
const TRANSACTION_META_MOCK = {
3744
id: BATCH_ID_MOCK,
@@ -40,6 +47,7 @@ const TRANSACTION_META_MOCK = {
4047
describe('Batch Utils', () => {
4148
const doesChainSupportEIP7702Mock = jest.mocked(doesChainSupportEIP7702);
4249
const getEIP7702SupportedChainsMock = jest.mocked(getEIP7702SupportedChains);
50+
const validateBatchRequestMock = jest.mocked(validateBatchRequest);
4351

4452
const isAccountUpgradedToEIP7702Mock = jest.mocked(
4553
isAccountUpgradedToEIP7702,
@@ -73,6 +81,7 @@ describe('Batch Utils', () => {
7381
addTransaction: addTransactionMock,
7482
getChainId: getChainIdMock,
7583
getEthQuery: GET_ETH_QUERY_MOCK,
84+
getInternalAccounts: GET_INTERNAL_ACCOUNTS_MOCK,
7685
messenger: MESSENGER_MOCK,
7786
request: {
7887
from: FROM_MOCK,
@@ -251,6 +260,16 @@ describe('Batch Utils', () => {
251260
rpcErrors.internal('Upgrade contract address not found'),
252261
);
253262
});
263+
264+
it('validates request', async () => {
265+
validateBatchRequestMock.mockImplementationOnce(() => {
266+
throw new Error('Validation Error');
267+
});
268+
269+
await expect(addTransactionBatch(request)).rejects.toThrow(
270+
'Validation Error',
271+
);
272+
});
254273
});
255274

256275
describe('isAtomicBatchSupported', () => {

‎packages/transaction-controller/src/utils/batch.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
getEIP7702SupportedChains,
1313
getEIP7702UpgradeContractAddress,
1414
} from './feature-flags';
15+
import { validateBatchRequest } from './validation';
1516
import type { TransactionController, TransactionControllerMessenger } from '..';
1617
import { projectLogger } from '../logger';
1718
import {
@@ -26,6 +27,7 @@ type AddTransactionBatchRequest = {
2627
addTransaction: TransactionController['addTransaction'];
2728
getChainId: (networkClientId: string) => Hex;
2829
getEthQuery: (networkClientId: string) => EthQuery;
30+
getInternalAccounts: () => Hex[];
2931
messenger: TransactionControllerMessenger;
3032
request: TransactionBatchRequest;
3133
};
@@ -50,10 +52,16 @@ export async function addTransactionBatch(
5052
const {
5153
addTransaction,
5254
getChainId,
55+
getInternalAccounts,
5356
messenger,
5457
request: userRequest,
5558
} = request;
5659

60+
validateBatchRequest({
61+
internalAccounts: getInternalAccounts(),
62+
request: userRequest,
63+
});
64+
5765
const { from, networkClientId, requireApproval, transactions } = userRequest;
5866

5967
log('Adding', userRequest);

‎packages/transaction-controller/src/utils/validation.test.ts‎

Lines changed: 88 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { ORIGIN_METAMASK } from '@metamask/controller-utils';
22
import { rpcErrors } from '@metamask/rpc-errors';
33

44
import {
5+
validateBatchRequest,
56
validateParamTo,
67
validateTransactionOrigin,
78
validateTxParams,
@@ -11,6 +12,7 @@ import type { TransactionParams } from '../types';
1112

1213
const FROM_MOCK = '0x1678a085c290ebd122dc42cba69373b5953b831d';
1314
const TO_MOCK = '0xfbb5595c18ca76bab52d66188e4ca50c7d95f77a';
15+
const ORIGIN_MOCK = 'test-origin';
1416

1517
describe('validation', () => {
1618
describe('validateTxParams', () => {
@@ -617,7 +619,7 @@ describe('validation', () => {
617619
await expect(
618620
validateTransactionOrigin({
619621
from: FROM_MOCK,
620-
origin: 'test-origin',
622+
origin: ORIGIN_MOCK,
621623
permittedAddresses: ['0x123', '0x456'],
622624
selectedAddress: '0x123',
623625
txParams: {} as TransactionParams,
@@ -633,7 +635,7 @@ describe('validation', () => {
633635
expect(
634636
await validateTransactionOrigin({
635637
from: FROM_MOCK,
636-
origin: 'test-origin',
638+
origin: ORIGIN_MOCK,
637639
permittedAddresses: ['0x123', FROM_MOCK],
638640
selectedAddress: '0x123',
639641
txParams: {} as TransactionParams,
@@ -645,7 +647,7 @@ describe('validation', () => {
645647
await expect(
646648
validateTransactionOrigin({
647649
from: FROM_MOCK,
648-
origin: 'test-origin',
650+
origin: ORIGIN_MOCK,
649651
permittedAddresses: [FROM_MOCK],
650652
selectedAddress: '0x123',
651653
txParams: {
@@ -663,7 +665,7 @@ describe('validation', () => {
663665
await expect(
664666
validateTransactionOrigin({
665667
from: FROM_MOCK,
666-
origin: 'test-origin',
668+
origin: ORIGIN_MOCK,
667669
permittedAddresses: [FROM_MOCK],
668670
selectedAddress: '0x123',
669671
txParams: {
@@ -683,7 +685,7 @@ describe('validation', () => {
683685
validateTransactionOrigin({
684686
from: FROM_MOCK,
685687
internalAccounts: [TO_MOCK],
686-
origin: 'test-origin',
688+
origin: ORIGIN_MOCK,
687689
selectedAddress: '0x123',
688690
txParams: {
689691
to: TO_MOCK,
@@ -701,7 +703,7 @@ describe('validation', () => {
701703
await validateTransactionOrigin({
702704
from: FROM_MOCK,
703705
internalAccounts: [TO_MOCK],
704-
origin: 'test-origin',
706+
origin: ORIGIN_MOCK,
705707
selectedAddress: '0x123',
706708
txParams: {
707709
to: TO_MOCK,
@@ -725,4 +727,84 @@ describe('validation', () => {
725727
);
726728
});
727729
});
730+
731+
describe('validateBatchRequest', () => {
732+
it('throws if external origin and any transaction target is internal account', () => {
733+
expect(() =>
734+
validateBatchRequest({
735+
internalAccounts: ['0x123', TO_MOCK],
736+
request: {
737+
from: FROM_MOCK,
738+
networkClientId: 'testNetworkClientId',
739+
origin: ORIGIN_MOCK,
740+
transactions: [
741+
{
742+
params: {
743+
to: '0xabc',
744+
},
745+
},
746+
{
747+
params: {
748+
to: TO_MOCK,
749+
},
750+
},
751+
],
752+
},
753+
}),
754+
).toThrow(
755+
rpcErrors.invalidParams(
756+
'External transactions to internal accounts are not supported',
757+
),
758+
);
759+
});
760+
761+
it('does not throw if no origin and any transaction target is internal account', () => {
762+
expect(() =>
763+
validateBatchRequest({
764+
internalAccounts: ['0x123', TO_MOCK],
765+
request: {
766+
from: FROM_MOCK,
767+
networkClientId: 'testNetworkClientId',
768+
transactions: [
769+
{
770+
params: {
771+
to: '0xabc',
772+
},
773+
},
774+
{
775+
params: {
776+
to: TO_MOCK,
777+
},
778+
},
779+
],
780+
},
781+
}),
782+
).not.toThrow();
783+
});
784+
785+
it('does not throw if internal origin and any transaction target is internal account', () => {
786+
expect(() =>
787+
validateBatchRequest({
788+
internalAccounts: ['0x123', TO_MOCK],
789+
request: {
790+
from: FROM_MOCK,
791+
networkClientId: 'testNetworkClientId',
792+
origin: ORIGIN_METAMASK,
793+
transactions: [
794+
{
795+
params: {
796+
to: '0xabc',
797+
},
798+
},
799+
{
800+
params: {
801+
to: TO_MOCK,
802+
},
803+
},
804+
],
805+
},
806+
}),
807+
).not.toThrow();
808+
});
809+
});
728810
});

‎packages/transaction-controller/src/utils/validation.ts‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { providerErrors, rpcErrors } from '@metamask/rpc-errors';
55
import { isStrictHexString, remove0x } from '@metamask/utils';
66

77
import { isEIP1559Transaction } from './utils';
8-
import type { Authorization } from '../types';
8+
import type { Authorization, TransactionBatchRequest } from '../types';
99
import {
1010
TransactionEnvelopeType,
1111
TransactionType,
@@ -247,6 +247,43 @@ export function validateParamTo(to?: string) {
247247
}
248248
}
249249

250+
/**
251+
* Validates a transaction batch request.
252+
*
253+
* @param options - Options bag.
254+
* @param options.internalAccounts - The internal accounts added to the wallet.
255+
* @param options.request - The batch request object.
256+
*/
257+
export function validateBatchRequest({
258+
internalAccounts,
259+
request,
260+
}: {
261+
internalAccounts: string[];
262+
request: TransactionBatchRequest;
263+
}) {
264+
const { origin } = request;
265+
const isExternal = origin && origin !== ORIGIN_METAMASK;
266+
267+
const transactionTargetsNormalized = request.transactions.map((tx) =>
268+
tx.params.to?.toLowerCase(),
269+
);
270+
271+
const internalAccountsNormalized = internalAccounts.map((account) =>
272+
account.toLowerCase(),
273+
);
274+
275+
if (
276+
isExternal &&
277+
transactionTargetsNormalized.some((target) =>
278+
internalAccountsNormalized.includes(target as string),
279+
)
280+
) {
281+
throw rpcErrors.invalidParams(
282+
'External transactions to internal accounts are not supported',
283+
);
284+
}
285+
}
286+
250287
/**
251288
* Validates input data for transactions.
252289
*

‎yarn.lock‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4264,7 +4264,7 @@ __metadata:
42644264
"@metamask/eth-block-tracker": ">=9"
42654265
"@metamask/gas-fee-controller": ^22.0.0
42664266
"@metamask/network-controller": ^22.0.0
4267-
"@metamask/remote-feature-flag-controller": ^1.3.0
4267+
"@metamask/remote-feature-flag-controller": ^1.5.0
42684268
languageName: unknown
42694269
linkType: soft
42704270

0 commit comments

Comments
 (0)