diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 2a749e2418..8ff7c5eb8f 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -11,9 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Export `decodeAuthorizationSignature` utility that decodes a 65-byte EIP-7702 authorization signature into RLP-canonical `r`, `s`, and `yParity` ([#8656](https://github.com/MetaMask/core/pull/8656)) - All `eth_sendRawTransaction` failures are prefixed `RPC submit:` for failure-surface attribution in error metrics +- Add optional `isInternal` flag to `addTransaction` and `addTransactionBatch`, persisted on `TransactionMeta` and `TransactionBatchMeta` ([#8633](https://github.com/MetaMask/core/pull/8633)) ### Changed +- **BREAKING:** Internal-only validations are now gated by `isInternal` instead of `origin === ORIGIN_METAMASK`; callers that relied on `ORIGIN_METAMASK` to skip them must pass `isInternal: true` ([#8633](https://github.com/MetaMask/core/pull/8633)) - Bump `@metamask/accounts-controller` from `^37.2.0` to `^38.0.0` ([#8665](https://github.com/MetaMask/core/pull/8665)) - Bump `@metamask/messenger` from `^1.1.1` to `^1.2.0` ([#8632](https://github.com/MetaMask/core/pull/8632)) - Bump `@metamask/network-controller` from `^30.0.1` to `^30.1.0` ([#8636](https://github.com/MetaMask/core/pull/8636)) diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index ea2b6da60d..1c53dc6808 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -1656,7 +1656,7 @@ describe('TransactionController', () => { await controller.addTransaction( { from: ACCOUNT_MOCK, to: ACCOUNT_MOCK, ...extraTxParamsToSet }, - { networkClientId: NETWORK_CLIENT_ID_MOCK }, + { isInternal: true, networkClientId: NETWORK_CLIENT_ID_MOCK }, ); expect(controller.state.transactions[0].txParams.type).toBe(type); @@ -3714,7 +3714,7 @@ describe('TransactionController', () => { ).rejects.toThrow('Batch ID already exists'); }); - it('does not throw if duplicate but internal origin', async () => { + it('does not throw if duplicate but isInternal', async () => { const { controller } = setupController({ options: { state: { @@ -3735,6 +3735,7 @@ describe('TransactionController', () => { await controller.addTransaction(txParams, { batchId: BATCH_ID_MOCK, + isInternal: true, networkClientId: NETWORK_CLIENT_ID_MOCK, }); }); diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index 83dbe27899..4566c5f6d2 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -1198,6 +1198,7 @@ export class TransactionController extends BaseController< excludeNativeTokenForFee, isGasFeeIncluded, isGasFeeSponsored, + isInternal = false, isStateOnly, method, nestedTransactions, @@ -1234,6 +1235,7 @@ export class TransactionController extends BaseController< data: txParams.data, from: txParams.from, internalAccounts, + isInternal, origin, permittedAddresses, txParams, @@ -1262,7 +1264,7 @@ export class TransactionController extends BaseController< (tx) => tx.batchId?.toLowerCase() === batchId?.toLowerCase(), ); - if (isDuplicateBatchId && origin && origin !== ORIGIN_METAMASK) { + if (isDuplicateBatchId && !isInternal) { throw new JsonRpcError( ErrorCode.DuplicateBundleId, 'Batch ID already exists', @@ -1272,6 +1274,7 @@ export class TransactionController extends BaseController< const dappSuggestedGasFees = this.#generateDappSuggestedGasFees( txParams, origin, + isInternal, ); const transactionType = @@ -1307,6 +1310,7 @@ export class TransactionController extends BaseController< ? {} : { excludeNativeTokenForFee }), isFirstTimeInteraction: undefined, + isInternal, isStateOnly, nestedTransactions, networkClientId, @@ -3621,8 +3625,9 @@ export class TransactionController extends BaseController< #generateDappSuggestedGasFees( txParams: TransactionParams, origin?: string, + isInternal?: boolean, ): DappSuggestedGasFees | undefined { - if (!origin || origin === ORIGIN_METAMASK) { + if (isInternal || !origin) { return undefined; } diff --git a/packages/transaction-controller/src/types.ts b/packages/transaction-controller/src/types.ts index 8881fb7b1f..2cd7efaa4d 100644 --- a/packages/transaction-controller/src/types.ts +++ b/packages/transaction-controller/src/types.ts @@ -325,6 +325,9 @@ export type TransactionMeta = { */ origin?: string; + /** Whether the transaction was added by trusted internal MetaMask code. */ + isInternal?: boolean; + /** * The original dapp proposed token approval amount before edit by user. */ @@ -613,6 +616,9 @@ export type TransactionBatchMeta = { */ origin?: string; + /** Whether the batch was added by trusted internal MetaMask code. */ + isInternal?: boolean; + /** * ID of the JSON-RPC request from DAPP. */ @@ -1866,6 +1872,9 @@ export type TransactionBatchRequest = { /** Origin of the request, such as a dApp hostname or `ORIGIN_METAMASK` if internal. */ origin?: string; + /** Whether the batch was added by trusted internal MetaMask code. */ + isInternal?: boolean; + /** Whether to overwrite existing EIP-7702 delegation with MetaMask contract. */ overwriteUpgrade?: boolean; @@ -2251,6 +2260,9 @@ export type AddTransactionOptions = { /** Origin of the transaction request, such as a dApp hostname. */ origin?: string; + /** Whether the transaction was added by trusted internal MetaMask code. */ + isInternal?: boolean; + /** Custom logic to publish the transaction. */ publishHook?: PublishHook; diff --git a/packages/transaction-controller/src/utils/batch.ts b/packages/transaction-controller/src/utils/batch.ts index 625fc6108c..0645744377 100644 --- a/packages/transaction-controller/src/utils/batch.ts +++ b/packages/transaction-controller/src/utils/batch.ts @@ -131,6 +131,7 @@ export async function addTransactionBatch( validateBatchRequest({ internalAccounts: getInternalAccounts(), + isInternal: transactionBatchRequest.isInternal, request: transactionBatchRequest, sizeLimit, }); @@ -296,6 +297,7 @@ async function addTransactionBatchWith7702( from, gasFeeToken, gasLimit7702, + isInternal, networkClientId, origin, overwriteUpgrade, @@ -438,6 +440,7 @@ async function addTransactionBatchWith7702( excludeNativeTokenForFee, isGasFeeIncluded, isGasFeeSponsored, + isInternal, nestedTransactions, networkClientId, origin, @@ -724,7 +727,7 @@ async function processTransactionWithHook( updateTransaction, } = request; - const { from, networkClientId, origin } = userRequest; + const { from, isInternal, networkClientId, origin } = userRequest; if (existingTransaction) { const { id, onPublish } = existingTransaction; @@ -797,6 +800,7 @@ async function processTransactionWithHook( assetsFiatValues, batchId, disableGasBuffer: true, + isInternal, networkClientId, origin, publishHook, @@ -941,6 +945,7 @@ async function prepareApprovalData({ const { from, + isInternal, origin, networkClientId, transactions: nestedTransactions, @@ -966,6 +971,7 @@ async function prepareApprovalData({ from, gas: gasLimit, id: batchId, + isInternal, networkClientId, origin, transactions: nestedTransactions, diff --git a/packages/transaction-controller/src/utils/gas-fees.test.ts b/packages/transaction-controller/src/utils/gas-fees.test.ts index 3a3912835c..b72854dc08 100644 --- a/packages/transaction-controller/src/utils/gas-fees.test.ts +++ b/packages/transaction-controller/src/utils/gas-fees.test.ts @@ -1,4 +1,3 @@ -import { ORIGIN_METAMASK } from '@metamask/controller-utils'; import type { NetworkClientId } from '@metamask/network-controller'; import type { TransactionControllerMessenger } from '../TransactionController'; @@ -506,9 +505,9 @@ describe('gas-fees', () => { ); }); - it('to custom if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and origin is metamask', async () => { + it('to custom if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and isInternal', async () => { updateGasFeeRequest.txMeta.txParams.gasPrice = GAS_HEX_MOCK; - updateGasFeeRequest.txMeta.origin = ORIGIN_METAMASK; + updateGasFeeRequest.txMeta.isInternal = true; await updateGasFees(updateGasFeeRequest); @@ -517,7 +516,7 @@ describe('gas-fees', () => { ); }); - it('to medium if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and origin not metamask', async () => { + it('to dappSuggested if request gas price but no request maxFeePerGas or maxPriorityFeePerGas and not isInternal', async () => { updateGasFeeRequest.txMeta.txParams.gasPrice = GAS_HEX_MOCK; updateGasFeeRequest.txMeta.origin = ORIGIN_MOCK; @@ -538,8 +537,8 @@ describe('gas-fees', () => { ); }); - it('to medium if origin is metamask', async () => { - updateGasFeeRequest.txMeta.origin = ORIGIN_METAMASK; + it('to medium if isInternal', async () => { + updateGasFeeRequest.txMeta.isInternal = true; await updateGasFees(updateGasFeeRequest); @@ -548,7 +547,7 @@ describe('gas-fees', () => { ); }); - it('to dappSuggested if origin is not metamask', async () => { + it('to dappSuggested if not isInternal', async () => { updateGasFeeRequest.txMeta.origin = ORIGIN_MOCK; await updateGasFees(updateGasFeeRequest); diff --git a/packages/transaction-controller/src/utils/gas-fees.ts b/packages/transaction-controller/src/utils/gas-fees.ts index b6b9899ed6..3598fac809 100644 --- a/packages/transaction-controller/src/utils/gas-fees.ts +++ b/packages/transaction-controller/src/utils/gas-fees.ts @@ -1,8 +1,4 @@ -import { - ORIGIN_METAMASK, - gweiDecToWEIBN, - toHex, -} from '@metamask/controller-utils'; +import { gweiDecToWEIBN, toHex } from '@metamask/controller-utils'; import type { FetchGasFeeEstimateOptions, GasFeeState, @@ -291,7 +287,7 @@ function getUserFeeLevel(request: GetGasFeeRequest): UserFeeLevel | undefined { !initialParams.maxPriorityFeePerGas && initialParams.gasPrice ) { - return txMeta.origin === ORIGIN_METAMASK + return txMeta.isInternal ? UserFeeLevel.CUSTOM : UserFeeLevel.DAPP_SUGGESTED; } @@ -313,7 +309,7 @@ function getUserFeeLevel(request: GetGasFeeRequest): UserFeeLevel | undefined { return UserFeeLevel.MEDIUM; } - if (txMeta.origin === ORIGIN_METAMASK) { + if (txMeta.isInternal) { return UserFeeLevel.MEDIUM; } diff --git a/packages/transaction-controller/src/utils/validation.test.ts b/packages/transaction-controller/src/utils/validation.test.ts index 006f25998d..4590acb700 100644 --- a/packages/transaction-controller/src/utils/validation.test.ts +++ b/packages/transaction-controller/src/utils/validation.test.ts @@ -1,4 +1,3 @@ -import { ORIGIN_METAMASK } from '@metamask/controller-utils'; import { rpcErrors } from '@metamask/rpc-errors'; import type { Hex } from '@metamask/utils'; @@ -792,6 +791,25 @@ describe('validation', () => { }), ).toBeUndefined(); }); + + it('does not throw if isInternal is true regardless of origin', async () => { + expect( + await validateTransactionOrigin({ + data: DATA_MOCK, + from: FROM_MOCK, + internalAccounts: [TO_MOCK], + isInternal: true, + origin: ORIGIN_MOCK, + permittedAddresses: ['0x123'], + selectedAddress: '0x123', + txParams: { + authorizationList: [], + to: TO_MOCK, + type: TransactionEnvelopeType.setCode, + } as TransactionParams, + }), + ).toBeUndefined(); + }); }); describe('validateParamTo', () => { @@ -842,7 +860,7 @@ describe('validation', () => { ).not.toThrow(); }); - it('does not throw if no origin and any transaction target is internal account with data', () => { + it('throws if no origin and any transaction target is internal account with data', () => { expect(() => validateBatchRequest({ ...VALIDATE_BATCH_REQUEST_MOCK, @@ -852,18 +870,19 @@ describe('validation', () => { origin: undefined, }, }), - ).not.toThrow(); + ).toThrow( + rpcErrors.invalidParams( + 'External calls to internal accounts cannot include data', + ), + ); }); - it('does not throw if internal origin and any transaction target is internal account with data', () => { + it('does not throw if internal and any transaction target is internal account with data', () => { expect(() => validateBatchRequest({ ...VALIDATE_BATCH_REQUEST_MOCK, internalAccounts: ['0x123', TO_MOCK], - request: { - ...VALIDATE_BATCH_REQUEST_MOCK.request, - origin: ORIGIN_METAMASK, - }, + isInternal: true, }), ).not.toThrow(); }); @@ -877,14 +896,11 @@ describe('validation', () => { ).toThrow(rpcErrors.invalidParams('Batch size cannot exceed 1. got: 2')); }); - it('does not throw if transaction count is internal and greater than limit', () => { + it('does not throw if internal and transaction count is greater than limit', () => { expect(() => validateBatchRequest({ ...VALIDATE_BATCH_REQUEST_MOCK, - request: { - ...VALIDATE_BATCH_REQUEST_MOCK.request, - origin: ORIGIN_METAMASK, - }, + isInternal: true, sizeLimit: 1, }), ).not.toThrow(); diff --git a/packages/transaction-controller/src/utils/validation.ts b/packages/transaction-controller/src/utils/validation.ts index 68dea00a73..31a9001cbf 100644 --- a/packages/transaction-controller/src/utils/validation.ts +++ b/packages/transaction-controller/src/utils/validation.ts @@ -1,5 +1,5 @@ import { Interface } from '@ethersproject/abi'; -import { ORIGIN_METAMASK, isValidHexAddress } from '@metamask/controller-utils'; +import { isValidHexAddress } from '@metamask/controller-utils'; import { abiERC20 } from '@metamask/metamask-eth-abis'; import { JsonRpcError, providerErrors, rpcErrors } from '@metamask/rpc-errors'; import type { Hex } from '@metamask/utils'; @@ -38,6 +38,7 @@ type GasFieldsToValidate = * @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.isInternal - Whether the transaction was added by trusted internal MetaMask code. * @param options.origin - The origin or source of the transaction. * @param options.permittedAddresses - The permitted accounts for the given origin. * @param options.selectedAddress - The currently selected Ethereum address in the wallet. @@ -49,6 +50,7 @@ export async function validateTransactionOrigin({ data, from, internalAccounts, + isInternal, origin, permittedAddresses, txParams, @@ -57,14 +59,13 @@ export async function validateTransactionOrigin({ data?: string; from: string; internalAccounts?: string[]; + isInternal?: boolean; origin?: string; permittedAddresses?: string[]; selectedAddress?: string; txParams: TransactionParams; type?: TransactionType; }): Promise { - const isInternal = !origin || origin === ORIGIN_METAMASK; - if (isInternal) { return; } @@ -258,27 +259,30 @@ export function validateParamTo(to?: string): void { * * @param options - Options bag. * @param options.internalAccounts - The internal accounts added to the wallet. + * @param options.isInternal - Whether the batch was added by trusted internal MetaMask code. * @param options.request - The batch request object. * @param options.sizeLimit - The maximum number of calls allowed in a batch request. */ export function validateBatchRequest({ internalAccounts, + isInternal, request, sizeLimit, }: { internalAccounts: string[]; + isInternal?: boolean; request: TransactionBatchRequest; sizeLimit: number; }): void { - const { origin } = request; - const isExternal = origin && origin !== ORIGIN_METAMASK; + if (isInternal) { + return; + } const internalAccountsNormalized = internalAccounts.map((account) => account.toLowerCase(), ); if ( - isExternal && request.transactions.some((nestedTransaction) => { const normalizedCallTo = nestedTransaction.params.to?.toLowerCase() as string; @@ -298,7 +302,7 @@ export function validateBatchRequest({ ); } - if (isExternal && request.transactions.length > sizeLimit) { + if (request.transactions.length > sizeLimit) { throw new JsonRpcError( ErrorCode.BundleTooLarge, `Batch size cannot exceed ${sizeLimit}. got: ${request.transactions.length}`,