feat: validate eth_sendTransaction / eth_signTransaction params#9482
Open
OGPoyraz wants to merge 18 commits into
Open
feat: validate eth_sendTransaction / eth_signTransaction params#9482OGPoyraz wants to merge 18 commits into
eth_sendTransaction / eth_signTransaction params#9482OGPoyraz wants to merge 18 commits into
Conversation
OGPoyraz
commented
Jul 13, 2026
| } from './methods/wallet-get-supported-execution-permissions'; | ||
| export * from './providerAsMiddleware'; | ||
| export * from './retryOnEmpty'; | ||
| export { validateTransactionParams } from './utils/validation'; |
Member
Author
There was a problem hiding this comment.
Extension/mobile PPOM middleware can gate params before their own normalizeTransactionParams call (which is where the WASM crash actually originates in clients).
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
jpuri
previously approved these changes
Jul 13, 2026
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
hilljessica8787-dot
approved these changes
Jul 14, 2026
jpuri
previously approved these changes
Jul 14, 2026
mcmire
reviewed
Jul 14, 2026
| throw rpcErrors.invalidInput(); | ||
| } | ||
|
|
||
| validateParams(params, TransactionParamsStruct); |
There was a problem hiding this comment.
Struct validation before size cap
Medium Severity
validateTransactionParams now runs full Superstruct validation before the JSON.stringify size check. Oversized but schema-shaped payloads (e.g. padded accessList entries) previously failed on length alone; they now pay for per-entry struct validation first, weakening the intended cheap early rejection.
Reviewed by Cursor Bugbot for commit 3009741. Configure here.
Member
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
mcmire
reviewed
Jul 14, 2026
mcmire
left a comment
mcmire
left a comment
Collaborator
There was a problem hiding this comment.
This looks good to me, but I would like another review from my team just to be safe.
…nTransaction params Reject requests whose top-level params contain keys outside the allowlisted transaction fields or nest beyond MAX_TRANSACTION_PARAM_DEPTH (10). Prevents downstream normalization / PPOM WASM from crashing with RangeError on deeply-nested junk fields and silently bypassing security scans. Mirrors the guardrail pattern introduced for eth_signTypedData_v4 in #8526. CONF-1662
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
| */ | ||
| export function validateTransactionParams(params: unknown): void { | ||
| if (Array.isArray(params)) { | ||
| throw rpcErrors.invalidInput(); |
Member
There was a problem hiding this comment.
I believe we use invalidParams elsewhere when parameters have an unexpected shape
Member
Author
There was a problem hiding this comment.
In fact, we don't need this at all as we are validating vs TransactionParamsStruct so removed this.
Member
There was a problem hiding this comment.
Also here, I wouldn't recommend we change any errors or messages at this stage if possible, since the validation we'd like to add is very limited.
|
|
||
| validateParams(params, TransactionParamsStruct); | ||
|
|
||
| if (JSON.stringify(params).length > MAX_TRANSACTION_PARAMS_SIZE_BYTES) { |
Member
There was a problem hiding this comment.
Suggested change
| if (JSON.stringify(params).length > MAX_TRANSACTION_PARAMS_SIZE_BYTES) { | |
| if (new TextEncoder().encode(JSON.stringify(params)).byteLength > MAX_TRANSACTION_PARAMS_SIZE_BYTES) { |
Technically this is more accurate (and also what we use in getJsonSize from utils). Though it is slower on mobile 😞
Member
Author
There was a problem hiding this comment.
Thanks for the ping changed this to use getJsonSize.
About performance concerns, I think this will be fine for benign requests as they are not massive.
Member
There was a problem hiding this comment.
getJsonSize has another performance penalty as it also validates that the input is JSON. We can skip that in this case. Maybe time to upstream: https://github.com/MetaMask/snaps/blob/a2ea600bc3cfa66a5e476fed4335d557c9cb2a4b/packages/snaps-utils/src/json.ts#L33-L37 😅
Member
Author
There was a problem hiding this comment.
Applied the suggestion 😅
| * Checks run in this order to guarantee we never recurse into hostile | ||
| * subtrees: | ||
| * | ||
| * 1. Top-level shape: params must be a plain object whose top-level keys |
Member
There was a problem hiding this comment.
Makes a lot of sense to be more cautions about validation here. Not necessarily a blocker, but I wonder if we've done any benchmarking at the cost (especially on mobile) of running x number of regexes on mobile?
Member
Author
There was a problem hiding this comment.
Unfortunately we haven't done any benchmarking because no data on Mixpanel of average size of benign dApp requests.
One plan in our mind is to check Sentinel state logs for submitted transactions but it will take time and we want to implement this as soon as possible to stop attack vectors.
Comment on lines
+254
to
+255
| accessList: optional(array(AccessListEntryStruct)), | ||
| authorizationList: optional(array(AuthorizationListEntryStruct)), |
Member
There was a problem hiding this comment.
Should we size limit these, just to some reasonable cap that cannot be exceeded in practice?
|
|
||
| const AccessListEntryStruct = object({ | ||
| address: HexAddressStruct, | ||
| storageKeys: array(StrictHexStruct), |
Member
There was a problem hiding this comment.
Same question as https://github.com/MetaMask/core/pull/9482/files#r3587566153
| nonce: optional(StrictHexStruct), | ||
| to: optional(HexAddressStruct), | ||
| type: optional(StrictHexStruct), | ||
| value: optional(StrictHexStruct), |
Member
There was a problem hiding this comment.
Do we not have coercion somewhere on fields like value, gasPrice, gasLimit etc somewhere? I thought dapps could get away with passing in numbers as well 🤔
Member
Author
There was a problem hiding this comment.
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.
matthewwalsh0
requested changes
Jul 15, 2026
| chainId: optional(StrictHexStruct), | ||
| data: optional(StrictHexStruct), | ||
| from: HexAddressStruct, | ||
| gas: optional(StrictHexStruct), |
Member
There was a problem hiding this comment.
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.
But size check may not enough alone?
Attackers are can bloat the request with tiny addition of object extending like {"a":{"a":{"a":{"a".... and even small size of the request can still overwhelm PPOM. Hence I was thinking implementing loose validation of shape.
But if the intent was just implementing size validation, I am totally fine to remove all.
Member
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 42f0c85. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Malicious dapps can send
eth_sendTransactionwith a valid-looking payload plus a deeply-nested junk field (e.g.test.b.b.b...~1200 levels). Downstream normalization in@metamask/transaction-controllerrecurses into these params and throwsRangeError: Maximum call stack size exceeded. When this crash happens inside the extension/mobile PPOM middleware it short-circuits the security-scan pipeline: the request never reaches the Security Alerts API, and the user sees the confirmation without a Blockaid scan.The same class of attack was fixed for
eth_signTypedData_v4in #8526 by rejecting requests with extraneous top-level keys. This PR applies the equivalent guardrail to transaction methods.Changes
validateTransactionParams(params)insrc/utils/validation.ts, exported from the package. Rejects withrpcErrors.invalidInput()when:paramsis not a plain object, orTransactionParams(accessList,authorizationList,chainId,data,from,gas,gasLimit,gasPrice,maxFeePerGas,maxPriorityFeePerGas,nonce,to,type,value), oraccessList/authorizationList, are ≤4 levels deep).eth_sendTransactionandeth_signTransactionhandlers increateWalletMiddleware, beforevalidateAndNormalizeKeyholder.index.tsso extension/mobile PPOM middleware can gate params before their ownnormalizeTransactionParamscall (which is where the WASM crash actually originates in clients).Design notes
invalidInput. Stripping silently would hide the attack pattern from telemetry.TransactionParams(estimateGasError,estimatedBaseFee,estimateSuggested,estimateUsed,gasUsed) — dapps should not be able to inject those.References
Checklist
Note
High Risk
Breaking RPC behavior on transaction methods at the wallet/security boundary; incorrect allowlisting or size limits could reject legitimate dapp traffic or leave attack paths open.
Overview
Adds breaking strict validation on
eth_sendTransactionandeth_signTransactionso malicious or malformed transaction params are rejected before account checks and downstream normalization.Introduces
validateTransactionParams(exported from the package) with a SuperstructTransactionParamsStructallowlist (fromrequired; optional dapp-facing fields includingaccessList/authorizationList). Unknown top-level keys fail schema validation without traversing their values, blocking deeply nested junk that could crash PPOM/normalization and skip security scans. After schema checks, serialized size overMAX_TRANSACTION_PARAMS_SIZE_BYTES(200 KB) throwsinvalidInput.Wallet middleware calls validation in both transaction handlers before
validateAndNormalizeKeyholder. Changelog and unit/integration tests cover extraneous keys, wrong types, padding attacks, and end-to-end RPC rejection.Reviewed by Cursor Bugbot for commit 2b91e13. Bugbot is set up for automated code reviews on this repo. Configure here.