feat: add canBeMalleable flag to broadcast/send responses#599
Open
jeremytsng wants to merge 4 commits intomainfrom
Open
feat: add canBeMalleable flag to broadcast/send responses#599jeremytsng wants to merge 4 commits intomainfrom
jeremytsng wants to merge 4 commits intomainfrom
Conversation
Surface a `canBeMalleable: boolean` on every snap response that returns
a post-broadcast txid, so consumers can detect when a transaction's id
may be rewritten by a third party before confirmation.
Computed from the source account's address type: only legacy P2PKH
(BIP44) keeps signatures in scriptSig and is therefore malleable. The
codebase's `p2sh` is BIP49 wrapped SegWit (sh(wpkh(...))) — signatures
live in the witness, so it is not malleable. Today every account is
P2WPKH, so the flag is always false; the implementation is in place
for when legacy support is added.
Wired through every consumer-facing txid path:
- AccountUseCases#broadcast (now the chokepoint, returns BroadcastResult)
- signPsbt / broadcastPsbt / sendTransfer (use-case layer)
- KeyringRequestHandler signPsbt / broadcastPsbt / sendTransfer
- RpcHandler executeSendFlow / signAndSend / confirmSend
- mapPsbtToTransaction (KeyringTransaction shape used by confirmSend)
KeyringRequestHandler asserts that signPsbt cannot return a txid
without the flag, mirroring the existing RpcHandler assertion. The
helper's exhaustive switch throws AssertionError on unknown
AddressType so a future variant cannot leak a non-boolean to consumers.
OpenRPC schemas updated for both sendTransactionResponse shapes and
confirmSend's KeyringTransaction return.
Limitation: the flag is computed from the snap account's address type,
which is correct for transactions the snap builds itself. For
externally-provided PSBTs broadcast via broadcastPsbt or
signPsbt({broadcast:true,fill:false}), foreign legacy inputs from
collaborative transactions could still produce a malleable txid; per-
input analysis is a follow-up.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 ad3413e. Configure here.
Cursor review noted the combined `!txid || canBeMalleable === undefined` guard threw 'Missing transaction ID' regardless of which condition fired, masking missing-flag protocol violations. Split the guard so each branch reports its own cause, matching the message used in KeyringRequestHandler.
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
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
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
Surfaces a
canBeMalleable: booleanon every snap response that returns a post-broadcasttxid, so consumers can detect when a transaction's id may be rewritten by a third party before confirmation.Current state. Every consumer-facing path that broadcasts a transaction returns the client-computed
txidimmediately after broadcast, with no signal about whether that id may change before confirmation. For legacy address types (P2PKH) the txid is malleable: a third party can rewrite the signature inscriptSigand produce a different valid txid for the same effective transaction. A dApp that trusts the returned txid for finality decisions before block confirmation can therefore lose track of its own transactions on legacy accounts.Solution. A new helper
canAccountTxidBeMalleated(addressType)inpackages/snap/src/entities/account.tsis the single source of truth for the rule:addressTypecanBeMalleablep2pkh(BIP44 legacy)truep2sh(BIP49 wrapped SegWit)falsep2wpkh(BIP84 native SegWit)falsep2wshfalsep2tr(Taproot)falseNote that
addressType: 'p2sh'in this codebase specifically means BIP49 nested SegWit (sh(wpkh(...))), not generic legacy P2SH — itsscriptSigis a fixed canonical push of the witness program and signatures live in the witness, so the txid is not malleable. Today every account is P2WPKH (enforced atKeyringHandler.tsfor v1), so the flag is alwaysfalsein production; the implementation is in place for the day legacy support is reconsidered. The exhaustiveswitchthrowsAssertionErroron an unknownAddressType, so a future variant cannot leak a non-boolean to consumers.Wired through every consumer-facing post-broadcast txid path:
AccountUseCases#broadcastis now the chokepoint: it returnsBroadcastResult { txid, canBeMalleable }, so the three public broadcast methods (signPsbt,broadcastPsbt,sendTransfer) can't return a txid without the flag.KeyringRequestHandlerpropagates the flag in itssignPsbt/broadcastPsbt/sendTransferresponses, and asserts on protocol violation (txid set without flag) — mirroring the existing assertion inRpcHandler.RpcHandlerpropagates the flag inexecuteSendFlow,signAndSend, and theconfirmSendpath.mapPsbtToTransaction(used byconfirmSend) now returnsKeyringTransactionWithMalleability, an additive widening of theKeyringTransactionshape.openrpc.jsondocumentscanBeMalleableon bothsendTransactionResponseschemas and onconfirmSend'sKeyringTransactionreturn.Limitation (out of scope for this PR). The flag is computed from the snap account's address type, which is correct for transactions the snap builds itself. For externally-provided PSBTs broadcast via
broadcastPsbtorsignPsbt({ broadcast: true, fill: false }), foreign legacy inputs from collaborative transactions could still produce a malleable txid even when the snap's account is P2WPKH. Per-input analysis is a follow-up — for the immediate audit concern (single-account broadcasts trusted by dApps) this PR is sufficient, and the limitation is documented in the JSDoc onBroadcastResult.References
Checklist
Note
Medium Risk
Changes public RPC/keyring response shapes for all post-broadcast transaction flows, which can break consumers that validate response schemas strictly. Logic is straightforward but touches core send/broadcast paths and adds new invariants that can throw on mismatched use-case outputs.
Overview
Surfaces a new
canBeMalleable: booleanflag anywhere the snap returns a post-broadcasttxid/transactionId, allowing clients to detect when a txid may be rewritten before confirmation (legacyp2pkhonly).This adds a single source of truth helper (
canAccountTxidBeMalleated) and threads the flag throughAccountUseCasesbroadcast results,RpcHandlersend responses, keyring submit responses (signPsbt/broadcastPsbt/sendTransfer), andconfirmSendtransaction mapping; handlers now assert if atxidis returned without the flag. OpenRPC docs, changelog, and integration/unit tests are updated accordingly.Reviewed by Cursor Bugbot for commit b781669. Bugbot is set up for automated code reviews on this repo. Configure here.