Skip to content

Commit

Permalink
Add validation for data property (#1701)
Browse files Browse the repository at this point in the history 
## Explanation

This PR aims to update the `TransactionController` to validate the
`data` property from `TransationParams`.

**Context**
The extension `TransactionController` validates the data field if not
empty by both ensuring it is a `string`, and using the
`@ethersproject/abi` and `human-standard-token-abi` packages to ensure
the contract data when parsed does not throw a `BUFFER_OVERRUN` error.

The changes in this PR add equivalent validation to the core
`TransactionController`.
## Changelog

### `@metamask/transaction-controller`

- **BREAKING**: update `validateTxParams` to validate `data` from
`TransactionParams` and throw when data is out-of-bounds.
  • Loading branch information
@vinistevam
vinistevam authored Oct 2, 2023
1 parent 8fcb2e4 commit 93ac28e
Show file tree
Hide file tree
Showing 5 changed files with 322 additions and 222 deletions.
2 changes: 1 addition & 1 deletion packages/transaction-controller/src/TransactionController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ import {
validateGasValues,
validateIfTransactionUnapproved,
validateMinimumIncrease,
validateTxParams,
ESTIMATE_GAS_ERROR,
} from './utils';
import { validateTxParams } from './validation';

export const HARDFORK = Hardfork.London;

Expand Down
148 changes: 0 additions & 148 deletions packages/transaction-controller/src/utils.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { rpcErrors } from '@metamask/rpc-errors';
import type { Transaction as NonceTrackerTransaction } from 'nonce-tracker/dist/NonceTracker';

import type {
Expand Down Expand Up @@ -72,153 +71,6 @@ describe('utils', () => {
});
});

describe('validateTxParams', () => {
it('should throw if no from address', () => {
expect(() => util.validateTxParams({} as any)).toThrow(
rpcErrors.invalidParams(
'Invalid "from" address: undefined must be a valid string.',
),
);
});

it('should throw if non-string from address', () => {
expect(() => util.validateTxParams({ from: 1337 } as any)).toThrow(
rpcErrors.invalidParams(
'Invalid "from" address: 1337 must be a valid string.',
),
);
});

it('should throw if invalid from address', () => {
expect(() => util.validateTxParams({ from: '1337' } as any)).toThrow(
rpcErrors.invalidParams(
'Invalid "from" address: 1337 must be a valid string.',
),
);
});

it('should throw if no data', () => {
expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '0x',
} as any),
).toThrow(
rpcErrors.invalidParams(
'Invalid "to" address: 0x must be a valid string.',
),
);

expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
} as any),
).toThrow(
rpcErrors.invalidParams(
'Invalid "to" address: undefined must be a valid string.',
),
);
});

it('should delete data', () => {
const transaction = {
data: 'foo',
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '0x',
};
util.validateTxParams(transaction);
expect(transaction.to).toBeUndefined();
});

it('should throw if invalid to address', () => {
expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '1337',
} as any),
).toThrow(
rpcErrors.invalidParams(
'Invalid "to" address: 1337 must be a valid string.',
),
);
});

it('should throw if value is invalid', () => {
expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '0x3244e191f1b4903970224322180f1fbbc415696b',
value: '133-7',
} as any),
).toThrow(
rpcErrors.invalidParams(
'Invalid "value": 133-7 is not a positive number.',
),
);

expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '0x3244e191f1b4903970224322180f1fbbc415696b',
value: '133.7',
} as any),
).toThrow(
rpcErrors.invalidParams(
'Invalid "value": 133.7 number must be denominated in wei.',
),
);

expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '0x3244e191f1b4903970224322180f1fbbc415696b',
value: 'hello',
} as any),
).toThrow(
rpcErrors.invalidParams(
'Invalid "value": hello number must be a valid number.',
),
);

expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '0x3244e191f1b4903970224322180f1fbbc415696b',
value: 'one million dollar$',
} as any),
).toThrow(
rpcErrors.invalidParams(
'Invalid "value": one million dollar$ number must be a valid number.',
),
);

expect(() =>
util.validateTxParams({
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
to: '0x3244e191f1b4903970224322180f1fbbc415696b',
value: '1',
} as any),
).not.toThrow();
});

it('throws if params specifies an EIP-1559 transaction but the current network does not support EIP-1559', () => {
expect(() =>
util.validateTxParams(
{
from: '0x3244e191f1b4903970224322180f1fbbc415696b',
maxFeePerGas: '2',
maxPriorityFeePerGas: '3',
} as any,
false,
),
).toThrow(
rpcErrors.invalidParams(
'Invalid transaction params: params specify an EIP-1559 transaction but the current network does not support EIP-1559',
),
);
});
});

describe('isEIP1559Transaction', () => {
it('should detect EIP1559 transaction', () => {
const tx: TransactionParams = { from: '' };
Expand Down
74 changes: 1 addition & 73 deletions packages/transaction-controller/src/utils.ts
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
import {
convertHexToDecimal,
isValidHexAddress,
} from '@metamask/controller-utils';
import { rpcErrors } from '@metamask/rpc-errors';
import { convertHexToDecimal } from '@metamask/controller-utils';
import { addHexPrefix, isHexString } from 'ethereumjs-util';
import type { Transaction as NonceTrackerTransaction } from 'nonce-tracker/dist/NonceTracker';

Expand Down Expand Up @@ -49,74 +45,6 @@ export function normalizeTxParams(txParams: TransactionParams) {
return normalizedTxParams;
}

/**
* Validates the transaction params for required properties and throws in
* the event of any validation error.
*
* @param txParams - Transaction params object to validate.
* @param isEIP1559Compatible - whether or not the current network supports EIP-1559 transactions.
*/
export function validateTxParams(
txParams: TransactionParams,
isEIP1559Compatible = true,
) {
if (
!txParams.from ||
typeof txParams.from !== 'string' ||
!isValidHexAddress(txParams.from)
) {
throw rpcErrors.invalidParams(
`Invalid "from" address: ${txParams.from} must be a valid string.`,
);
}

if (isEIP1559Transaction(txParams) && !isEIP1559Compatible) {
throw rpcErrors.invalidParams(
'Invalid transaction params: params specify an EIP-1559 transaction but the current network does not support EIP-1559',
);
}

if (txParams.to === '0x' || txParams.to === undefined) {
if (txParams.data) {
delete txParams.to;
} else {
throw rpcErrors.invalidParams(
`Invalid "to" address: ${txParams.to} must be a valid string.`,
);
}
} else if (txParams.to !== undefined && !isValidHexAddress(txParams.to)) {
throw rpcErrors.invalidParams(
`Invalid "to" address: ${txParams.to} must be a valid string.`,
);
}

if (txParams.value !== undefined) {
const value = txParams.value.toString();
if (value.includes('-')) {
throw rpcErrors.invalidParams(
`Invalid "value": ${value} is not a positive number.`,
);
}

if (value.includes('.')) {
throw rpcErrors.invalidParams(
`Invalid "value": ${value} number must be denominated in wei.`,
);
}
const intValue = parseInt(txParams.value, 10);
const isValid =
Number.isFinite(intValue) &&
!Number.isNaN(intValue) &&
!isNaN(Number(value)) &&
Number.isSafeInteger(intValue);
if (!isValid) {
throw rpcErrors.invalidParams(
`Invalid "value": ${value} number must be a valid number.`,
);
}
}
}

/**
* Checks if a transaction is EIP-1559 by checking for the existence of
* maxFeePerGas and maxPriorityFeePerGas within its parameters.
Expand Down
Loading

0 comments on commit 93ac28e

Please sign in to comment.