From 7154b2fa05816eaa3197f95221bdcd04634c9a68 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Tue, 5 May 2026 14:28:58 +0100 Subject: [PATCH] fix: reject transactions that would deploy an empty contract When a transaction has no recipient (`to` is empty string, `'0x'`, or `undefined`) and no real bytecode (`data` is empty or `'0x'`), the TransactionController would silently classify it as a contract deployment and broadcast it. Two places contributed: - `validateParamRecipient` treated `data === '0x'` as real bytecode because `'0x'` is a non-empty string and therefore truthy. - `determineTransactionType` had the same `'0x'` truthiness bug. Now: - `validateParamRecipient` requires `data.length > 2` for the bytecode to count as real, treats `to === ''` as missing alongside `'0x'` and `undefined`, and rejects with a more descriptive error message. - `determineTransactionType` only returns `deployContract` when there is real bytecode. --- packages/transaction-controller/CHANGELOG.md | 4 ++ .../src/utils/transaction-type.test.ts | 30 +++++++- .../src/utils/transaction-type.ts | 4 +- .../src/utils/validation.test.ts | 72 +++++++++++++++++-- .../src/utils/validation.ts | 27 +++++-- 5 files changed, 124 insertions(+), 13 deletions(-) diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 2d2da9c929..e5dbfcb379 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Trigger the first-time-interaction warning correctly for `safeTransferFrom` token transfers by including `TransactionType.tokenMethodSafeTransferFrom` in the effective-recipient decoding logic ([#8723](https://github.com/MetaMask/core/pull/8723)) +### Fixed + +- Reject transactions with a missing `to` and empty `data` to prevent accidental empty contract deployments ([#8744](https://github.com/MetaMask/core/pull/8744)) + ## [65.2.0] ### Added diff --git a/packages/transaction-controller/src/utils/transaction-type.test.ts b/packages/transaction-controller/src/utils/transaction-type.test.ts index a5e33682cd..2b6b57bee3 100644 --- a/packages/transaction-controller/src/utils/transaction-type.test.ts +++ b/packages/transaction-controller/src/utils/transaction-type.test.ts @@ -165,7 +165,7 @@ describe('determineTransactionType', () => { }); }); - it('returns a contract deployment type when "to" is falsy and there is data', async () => { + it('returns a contract deployment type when "to" is falsy and there is real bytecode', async () => { const result = await determineTransactionType( { ...txParams, @@ -180,6 +180,34 @@ describe('determineTransactionType', () => { }); }); + it('does NOT classify as deployContract when "to" is falsy but data is "0x" (empty bytecode)', async () => { + jest.mocked(rpcRequest).mockResolvedValue('0x'); + + const result = await determineTransactionType( + { + ...txParams, + to: '', + data: '0x', + }, + { messenger: messengerMock, networkClientId: networkClientIdMock }, + ); + expect(result.type).not.toBe(TransactionType.deployContract); + }); + + it('does NOT classify as deployContract when "to" is undefined but data is "0x"', async () => { + jest.mocked(rpcRequest).mockResolvedValue('0x'); + + const result = await determineTransactionType( + { + ...txParams, + to: undefined, + data: '0x', + }, + { messenger: messengerMock, networkClientId: networkClientIdMock }, + ); + expect(result.type).not.toBe(TransactionType.deployContract); + }); + it('returns a simple send type with a 0x getCodeResponse when there is data, but the "to" address is not a contract address', async () => { jest.mocked(rpcRequest).mockResolvedValue('0x'); diff --git a/packages/transaction-controller/src/utils/transaction-type.ts b/packages/transaction-controller/src/utils/transaction-type.ts index 1832354a1d..85ccad46cc 100644 --- a/packages/transaction-controller/src/utils/transaction-type.ts +++ b/packages/transaction-controller/src/utils/transaction-type.ts @@ -41,7 +41,9 @@ export async function determineTransactionType( ): Promise { const { data, to } = txParams; - if (data && !to) { + const hasRealBytecode = Boolean(data && data !== '0x' && data.length > 2); + + if (hasRealBytecode && !to) { return { type: TransactionType.deployContract, getCodeResponse: undefined }; } diff --git a/packages/transaction-controller/src/utils/validation.test.ts b/packages/transaction-controller/src/utils/validation.test.ts index 006f25998d..c9a2f53d64 100644 --- a/packages/transaction-controller/src/utils/validation.test.ts +++ b/packages/transaction-controller/src/utils/validation.test.ts @@ -77,7 +77,11 @@ describe('validation', () => { ); }); - it('should throw if no data', () => { + it('should throw if to is missing and no real bytecode', () => { + const expectedError = rpcErrors.invalidParams( + 'Invalid "to" address: must be specified for transactions without contract deployment bytecode.', + ); + expect(() => validateTxParams({ from: FROM_MOCK, @@ -85,7 +89,7 @@ describe('validation', () => { // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any), - ).toThrow(rpcErrors.invalidParams('Invalid "to" address.')); + ).toThrow(expectedError); expect(() => validateTxParams({ @@ -93,12 +97,60 @@ describe('validation', () => { // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any), - ).toThrow(rpcErrors.invalidParams('Invalid "to" address.')); + ).toThrow(expectedError); + }); + + it('should throw if to is empty string and no real bytecode', () => { + expect(() => + validateTxParams({ + from: FROM_MOCK, + to: '' as Hex, + data: '0x', + value: '0x1', + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any), + ).toThrow( + rpcErrors.invalidParams( + 'Invalid "to" address: must be specified for transactions without contract deployment bytecode.', + ), + ); }); - it('should delete data', () => { + it('should throw if to is empty string and data is "0x" (would deploy empty contract)', () => { + expect(() => + validateTxParams({ + from: FROM_MOCK, + to: '' as Hex, + data: '0x', + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any), + ).toThrow( + rpcErrors.invalidParams( + 'Invalid "to" address: must be specified for transactions without contract deployment bytecode.', + ), + ); + }); + + it('should throw if to is undefined and data is "0x" (would deploy empty contract)', () => { + expect(() => + validateTxParams({ + from: FROM_MOCK, + data: '0x', + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any), + ).toThrow( + rpcErrors.invalidParams( + 'Invalid "to" address: must be specified for transactions without contract deployment bytecode.', + ), + ); + }); + + it('should remove "to" when missing and data has real bytecode (legitimate deployment)', () => { const transaction = { - data: 'foo', + data: '0x608060405234', from: TO_MOCK, to: '0x', }; @@ -106,6 +158,16 @@ describe('validation', () => { expect(transaction.to).toBeUndefined(); }); + it('should remove "to" when empty string and data has real bytecode', () => { + const transaction = { + data: '0x608060405234', + from: TO_MOCK, + to: '' as Hex, + }; + validateTxParams(transaction); + expect(transaction.to).toBeUndefined(); + }); + it('should throw if invalid to address', () => { expect(() => validateTxParams({ diff --git a/packages/transaction-controller/src/utils/validation.ts b/packages/transaction-controller/src/utils/validation.ts index 68dea00a73..a523fb2e82 100644 --- a/packages/transaction-controller/src/utils/validation.ts +++ b/packages/transaction-controller/src/utils/validation.ts @@ -205,16 +205,31 @@ function validateParamValue(value?: string): void { * * @param txParams - The transaction parameters object to validate. * @throws Throws an error if the recipient address is invalid: - * - If the recipient address is an empty string ('0x') or undefined and the transaction contains data, - * the "to" field is removed from the transaction parameters. - * - If the recipient address is not a valid hexadecimal Ethereum address, an error is thrown. + * - If the recipient address is missing (empty string, '0x', or undefined) and the + * transaction does not contain real bytecode (data must be longer than `0x`), + * an error is thrown. This prevents accidental contract deployments with empty + * `to` and empty `data` from locking funds. + * - If the recipient address is missing and the transaction contains real + * bytecode (data longer than `0x`), the "to" field is removed from the + * transaction parameters (legitimate contract deployment). + * - If the recipient address is not a valid hexadecimal Ethereum address, an + * error is thrown. */ function validateParamRecipient(txParams: TransactionParams): void { - if (txParams.to === '0x' || txParams.to === undefined) { - if (txParams.data) { + const isMissingRecipient = + txParams.to === '0x' || txParams.to === '' || txParams.to === undefined; + + if (isMissingRecipient) { + const hasRealBytecode = Boolean( + txParams.data && txParams.data !== '0x' && txParams.data.length > 2, + ); + + if (hasRealBytecode) { delete txParams.to; } else { - throw rpcErrors.invalidParams(`Invalid "to" address.`); + throw rpcErrors.invalidParams( + `Invalid "to" address: must be specified for transactions without contract deployment bytecode.`, + ); } } else if (txParams.to !== undefined && !isValidHexAddress(txParams.to)) { throw rpcErrors.invalidParams(`Invalid "to" address.`);