Skip to content

feat: warn on self-sends in sendTransfer confirmation#607

Open
jeremytsng wants to merge 3 commits intomainfrom
fix/sendtransfer-confirmation-check-self-sends
Open

feat: warn on self-sends in sendTransfer confirmation#607
jeremytsng wants to merge 3 commits intomainfrom
fix/sendtransfer-confirmation-check-self-sends

Conversation

@jeremytsng
Copy link
Copy Markdown

@jeremytsng jeremytsng commented May 6, 2026

Explanation

The dApp-initiated sendTransfer keyring flow built ConfirmSendFormContext without checking whether the recipient address belonged to the sending account. The companion signPsbt flow already runs account.isMine on each output (so it can label change vs. recipient), but sendTransfer did not — so UnifiedSendFormView rendered self-sends as ordinary outgoing transfers with no indication that the funds were going back to the same wallet.

This change brings sendTransfer to parity with signPsbt:

  • JSXConfirmationRepository.insertSendTransfer derives isMine from the recipient's script_pubkey (BdkAddress.from_string(recipient.address, account.network).script_pubkeyaccount.isMine(...)), with a try/catch defaulting to false on parse failure — same defensive pattern used in insertSignPsbt.
  • UnifiedSendFormView renders a warning Banner above the estimated changes when isMine === true ("Sending to your own address — the recipient address belongs to this wallet. The amount will stay in your account, but network fees still apply.").
  • The same isMine derivation is added to the in-app SendFlowUseCases.confirmSendFlow path so both producers of ConfirmSendFormContext (the dApp keyring path and the in-app send flow) behave consistently.
  • isMine is a required field on ConfirmSendFormContext so the type system enforces parity for any future producer.
  • The two new locale keys are added to all 16 locale files (English text as a placeholder for non-EN locales until translations land), since the translator falls back per-file rather than per-key.

A note on visual treatment: signPsbt's confirmation labels per-output rows as "Change" when isMine is true. sendTransfer enforces exactly one recipient, so labelling the only output as "Change" would be misleading — it's the actual recipient. A top-level Banner is the closest equivalent to the "alert" the suggestion describes, and is functionally equivalent (and arguably more prominent) than per-row labelling.

References

Closes the security-audit suggestion: "sendTransfer Confirmation Does Not Check for Self-Sends".

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Changes the send-confirmation context and UI to detect and warn on self-sends, which could affect transaction confirmation behavior if address parsing or ownership checks are wrong. Logic is guarded with try/catch and covered by new tests, limiting blast radius to confirmation UX.

Overview
Adds self-send detection to the send confirmation flow by deriving a new required ConfirmSendFormContext.isMine flag from the recipient address’ script_pubkey and account.isMine(...) (with a safe fallback to false on parse errors).

Updates UnifiedSendFormView to show a warning Banner when isMine is true, and introduces new i18n message keys across messages.json and all locale files (plus a changelog entry). Unit tests are expanded to cover isMine true/false and invalid-address scenarios in both the dApp insertSendTransfer path and the in-app confirmSendFlow path.

Reviewed by Cursor Bugbot for commit b198577. Bugbot is set up for automated code reviews on this repo. Configure here.

jeremytsng added 2 commits May 6, 2026 17:22
The dApp-initiated sendTransfer flow built ConfirmSendFormContext
without checking whether the recipient address belonged to the
sending account, so UnifiedSendFormView rendered self-sends as
ordinary outgoing transfers. The signPsbt flow already runs
account.isMine on each output. Bring sendTransfer to parity:
derive isMine from the recipient's script_pubkey in
JSXConfirmationRepository and surface a warning banner in the
confirmation modal when it's true.
Address codex audit findings on the previous commit:

- isMine was optional and only populated by the dApp keyring path; the
  in-app SendFlowUseCases.confirmSendFlow renders the same
  UnifiedSendFormView and was missing the warning. Make isMine required
  on ConfirmSendFormContext and derive it from the recipient script in
  both producers.
- The new self-send copy was only added to en.json, so non-English
  locales would render the raw {key} placeholder. Add the keys to every
  locale file with the English text as a placeholder until translations
  land.
- Adjust the warning copy to acknowledge that self-sends still pay
  network fees.
- Regenerate the snap manifest shasum after the source changes.
@jeremytsng jeremytsng requested a review from a team as a code owner May 6, 2026 10:22
@jeremytsng jeremytsng changed the title fix: warn on self-sends in sendTransfer confirmation feat: warn on self-sends in sendTransfer confirmation May 6, 2026
@jeremytsng
Copy link
Copy Markdown
Author

@metamaskbot publish-preview

@Battambang
Copy link
Copy Markdown
Contributor

Please, consider:

  • Create the preview-build in the PR and testing it with a bump in the MM client.
  • Add in the PR the screenshot of the dialog box.
  • Update the changelod.MD about this user facing change.
  • Have QA acceptance feedback for merge to main.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/bitcoin-wallet-snap": "1.10.1-preview-d973a4a"
}

Also regenerates snap.manifest.json shasum to match the current
bundle output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants