-
-
Notifications
You must be signed in to change notification settings - Fork 290
feat: validate eth_sendTransaction / eth_signTransaction params
#9482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 16 commits
e4570f5
8a62471
c138500
42e4bee
186f262
4f96cf5
a5db667
fa3e763
77fd64e
d6b900c
2cdbbcc
57e5c1c
30283ac
0fb9263
97c8b4b
42f0c85
ced4991
2b91e13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| import { TYPED_MESSAGE_SCHEMA } from '@metamask/eth-sig-util'; | ||
| import { providerErrors, rpcErrors } from '@metamask/rpc-errors'; | ||
| import type { Struct, StructError } from '@metamask/superstruct'; | ||
| import { validate } from '@metamask/superstruct'; | ||
| import { array, object, optional, validate } from '@metamask/superstruct'; | ||
| import type { Hex } from '@metamask/utils'; | ||
| import { getJsonSize, HexAddressStruct, StrictHexStruct } from '@metamask/utils'; | ||
|
|
||
| import type { WalletMiddlewareContext } from '../wallet'; | ||
| import { parseTypedMessage } from './normalize'; | ||
|
|
@@ -234,3 +235,70 @@ export function validateTypedMessageKeys(data: string): void { | |
| } | ||
| } | ||
| } | ||
|
|
||
| const AccessListEntryStruct = object({ | ||
| address: HexAddressStruct, | ||
| storageKeys: array(StrictHexStruct), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as https://github.com/MetaMask/core/pull/9482/files#r3587566153 |
||
| }); | ||
|
|
||
| const AuthorizationListEntryStruct = object({ | ||
| address: HexAddressStruct, | ||
| chainId: StrictHexStruct, | ||
| nonce: StrictHexStruct, | ||
| r: optional(StrictHexStruct), | ||
| s: optional(StrictHexStruct), | ||
| yParity: optional(StrictHexStruct), | ||
| }); | ||
|
|
||
| export const TransactionParamsStruct = object({ | ||
| accessList: optional(array(AccessListEntryStruct)), | ||
| authorizationList: optional(array(AuthorizationListEntryStruct)), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we size limit these, just to some reasonable cap that cannot be exceeded in practice? |
||
| chainId: optional(StrictHexStruct), | ||
| data: optional(StrictHexStruct), | ||
| from: HexAddressStruct, | ||
| gas: optional(StrictHexStruct), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the confusion Goktug, I was suggesting we add a superstruct schema as a mechanism to define valid fields. But I definitely would not recommend we add any additional validations in this PR, in the form of specific property types, as the scope of the change is specifically for total request size. So should all these just be loose properties as they were before, at least for now? So the only net change is the size validation?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But size check may not enough alone? Attackers are can bloat the request with tiny addition of object extending like But if the intent was just implementing size validation, I am totally fine to remove all.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No I think the schema is a good idea to define what fields are allowed, but I'd be nervous to define strict types for those fields in this PR, as the risk to dApps is big given normalization etc. |
||
| gasLimit: optional(StrictHexStruct), | ||
| gasPrice: optional(StrictHexStruct), | ||
| maxFeePerGas: optional(StrictHexStruct), | ||
| maxPriorityFeePerGas: optional(StrictHexStruct), | ||
| nonce: optional(StrictHexStruct), | ||
| to: optional(HexAddressStruct), | ||
| type: optional(StrictHexStruct), | ||
| value: optional(StrictHexStruct), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not have coercion somewhere on fields like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, but with this PR we're only focused on the shape of the request and its size - we're not aiming to fully implement strict validations yet. More rules will likely follow in future work. |
||
| }); | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| // Upper bound derived from the largest valid eth_sendTransaction payload: | ||
| // EIP-3860 caps initcode at 49,152 bytes → hex-encoded in 'data' field ≈ 98 KB of JSON. | ||
| // 200 KB is ~2× that ceiling, giving clear headroom above any protocol-legal | ||
| // transaction while blocking the padding attacks this cap defends against. | ||
| // TODO(CONF-1662): tighten once P99 production data is available. | ||
| export const MAX_TRANSACTION_PARAMS_SIZE_BYTES = 200 * 1024; | ||
|
|
||
| /** | ||
| * Validates `eth_sendTransaction` / `eth_signTransaction` params against the | ||
| * standard transaction schema and rejects payloads whose serialized size | ||
| * exceeds `MAX_TRANSACTION_PARAMS_SIZE_BYTES`. | ||
| * | ||
| * Guards against two attack shapes: | ||
| * - Structural: extraneous top-level keys or ill-typed fields (e.g. | ||
| * `{ from, to, test: { b: { b: ... × 1200 } } }`) that would crash | ||
| * downstream normalization / PPOM WASM with `RangeError: Maximum call | ||
| * stack size exceeded`, silently bypassing security checks. Superstruct's | ||
| * `object()` rejects unknown keys by name without accessing their values, | ||
| * so hostile nested subtrees are never traversed. | ||
| * - Size: valid-shaped but oversized payloads (e.g. `data` padded with | ||
| * millions of hex zeros) that exhaust memory in downstream code. | ||
| * | ||
| * @param params - The transaction params object supplied by the dapp. | ||
| * @throws rpcErrors.invalidParams() if params is an array or exceeds the | ||
| * serialized size limit. | ||
| * @throws rpcErrors.invalidInput() if params fails schema validation | ||
| * (wrong type, extraneous top-level key, or malformed nested field). | ||
| */ | ||
| export function validateTransactionParams(params: unknown): void { | ||
| validateParams(params, TransactionParamsStruct); | ||
|
cursor[bot] marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Struct validation before size capMedium Severity
Reviewed by Cursor Bugbot for commit 3009741. Configure here.
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| if (getJsonSize(params) > MAX_TRANSACTION_PARAMS_SIZE_BYTES) { | ||
| throw rpcErrors.invalidInput(); | ||
| } | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extension/mobile PPOM middleware can gate params before their own
normalizeTransactionParamscall (which is where the WASM crash actually originates in clients).