Skip to content

chore: bump @metamask/phishing-controller to 17.3.1 - #34158

Merged
adonesky1 merged 2 commits into
mainfrom
bump/phishing-controller-17.3.1
Aug 3, 2026
Merged

adonesky1 merged 2 commits into
mainfrom
bump/phishing-controller-17.3.1

Conversation

@adonesky1

@adonesky1 adonesky1 commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Description

Bumps @metamask/phishing-controller from ^17.3.0 to ^17.3.1 to pick up the address-poisoning fix from MetaMask/core#9699.

Before that fix, the known-recipients list used to detect address poisoning took txParams.to from confirmed transactions. For ERC-20/721/1155 transfers txParams.to is the token contract, not the address receiving the tokens, so two things were wrong:

  • a lookalike of the real token recipient did not trigger address-poisoning detection
  • a lookalike of the token contract address did trigger it, which is meaningless noise

17.3.1 decodes the actual recipient from calldata via the new getEffectiveRecipient utility, so token transfers now contribute the real recipient. Plain sends and other contract interactions are unchanged.

Why @metamask/transaction-controller moves to 69.4.0

phishing-controller@17.3.1 declares @metamask/transaction-controller: ^69.4.0, because getEffectiveRecipient is exported from 69.4.0.

This repo pins @metamask/transaction-controller to an exact version in resolutions, which collapses every range in the tree onto that one version. That pin has to move as well — leaving it at 69.3.0 would silently resolve phishing-controller@17.3.1 against 69.3.0, where getEffectiveRecipient exists only as an internal helper inside first-time-interaction and is not exported from the package entry point. The import would be undefined and would throw on every confirmed transaction. So this PR moves the resolutions pin and the declared range together, and the lockfile still has exactly one transaction-controller entry.

Two notes for reviewers on the dependency graph:

  • network-controller is unaffected. transaction-controller@69.4.0 declares network-controller: ^35.0.0, but this repo's resolutions pin holds it at 34.0.0 and the lockfile still has exactly one 34.0.0 entry. That pin is safe here: the published 69.3.0 and 69.4.0 bundles are byte-identical apart from the getEffectiveRecipient export, an optional strategy?: string field on MetamaskPayMetadata, and the moved helper file. No compiled module in 69.4.0 references network-controller, NetworkClient, BuiltInNetworkClientId, or analyticsOptions, so its ^35.0.0 range is just the mechanical release-wide bump from Release/1163.0.0 core#9735 rather than an adoption of v35 APIs. accounts-controller is likewise held at 39.0.3 by its existing pin.
  • remote-feature-flag-controller@5.0.0 is added next to the existing 4.2.2. These can't be deduped, since ^4.2.x cannot accept 5.0.0. transaction-controller@69.4.0 only references it from type declarations (dist/TransactionController.d.cts) and never from runtime JS, so nothing new gets pulled into the bundle. Happy to add a resolutions pin instead if the team would rather avoid the duplicate entry.

A scoped yarn dedupe collapses the core-backend, gas-fee-controller, and polling-controller ranges that 69.4.0 nudges forward, so those don't gain parallel copies either.

Changelog

CHANGELOG entry: Fixed address-poisoning detection so lookalikes of the actual recipient of a token transfer are flagged, and lookalikes of the token contract address are no longer flagged.

Related issues

Fixes:

Manual testing steps

Feature: address-poisoning detection for token transfer recipients

Scenario: user is warned about a lookalike of a real token recipient
Given the user has previously sent an ERC-20 token to an address and that transaction is confirmed

When user starts a new transfer of that token and enters a recipient that matches the previous recipient's first four and last four hex characters but differs in the middle
Then the address-poisoning warning is shown

Scenario: user is not warned about a lookalike of the token contract
Given the user has previously sent an ERC-20 token and that transaction is confirmed

When user enters a recipient that is a lookalike of the token contract address
Then no address-poisoning warning is shown, because the contract is no longer treated as a known recipient

Scenario: plain sends and contract interactions are unchanged
Given the user has a confirmed plain ETH transfer and a confirmed generic contract interaction

When user enters a lookalike of either of those recipients
Then the address-poisoning warning is still shown

Screenshots/Recordings

Before

A lookalike of the real ERC-20 recipient produced no warning, while a lookalike of the token contract produced one.

After

A lookalike of the real ERC-20 recipient produces a warning; a lookalike of the token contract does not.

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Medium Risk
Touches security-sensitive address-poisoning logic and requires the transaction-controller pin to match; runtime impact is limited to dependency behavior, but incorrect resolution would break confirmed-tx handling.

Overview
Bumps @metamask/phishing-controller from ^17.3.0 to ^17.3.1 so address-poisoning detection uses the real token transfer recipient (via getEffectiveRecipient) instead of txParams.to, which for ERC-20/721/1155 is the token contract. Lookalikes of the actual recipient should warn; lookalikes of the contract should not.

Because 17.3.1 depends on an exported getEffectiveRecipient from @metamask/transaction-controller, this PR also moves the resolutions pin and dependency from 69.3.0 to 69.4.0 and refreshes yarn.lock (including transitive bumps such as remote-feature-flag-controller@5.0.0 alongside the existing 4.2.2). No application source files change—behavior comes from the upgraded packages.

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

@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamask-ci metamask-ci Bot added the team-wallet-integrations Wallet Integrations team label Jul 31, 2026
@metamask-ci

metamask-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Related issues section is empty. Add Fixes: #123 / Closes: <URL> / Refs: <Jira key>, or write a short rationale after the colon.
  • Pre-merge author checklist has only 5 of the required 8 items. Every checklist row must be present and consciously checked — do not delete rows.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@socket-security

socket-security Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatednpm/​@​metamask/​phishing-controller@​17.3.0 ⏵ 17.3.1991007898 +1100
Updatednpm/​@​metamask/​transaction-controller@​69.3.0 ⏵ 69.4.098 +110081 +1100 +1100

View full report

@socket-security

socket-security Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Warning

MetaMask internal reviewing guidelines:

  • Do not ignore-all
  • Each alert has instructions on how to review if you don't know what it means. If lost, ask your Security Liaison or the supply-chain group
  • Copy-paste ignore lines for specific packages or a group of one kind with a note on what research you did to deem it safe.
    @SocketSecurity ignore npm/PACKAGE@VERSION
Action Severity Alert  (click "▶" to expand/collapse)
Warn Low
Potential code anomaly (AI signal): npm @metamask/transaction-controller is 75.0% likely to have a medium risk anomaly

Notes: The code performs straightforward signature verification using ethers.js, returning true when the recovered signer matches the provided publicKey. While generally safe, the silent catch and potential mismatch between data formatting and signing process should be addressed to avoid silent failures. Overall, a benign utility with moderate input-format sensitivity.

Confidence: 0.75

Severity: 0.50

From: package.json → npm/@metamask/transaction-controller@69.4.0

ℹ Read more on: This package | This alert | What is an AI-detected potential code anomaly?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: An AI system found a low-risk anomaly in this package. It may still be fine to use, but you should check that it is safe before proceeding.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/@metamask/transaction-controller@69.4.0. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@github-actions github-actions Bot added the risk:high AI analysis: high risk label Jul 31, 2026
Picks up the address-poisoning fix from MetaMask/core#9699, so known
recipients use the token recipient decoded from calldata rather than the
token contract address for confirmed ERC-20/721/1155 transfers.

`@metamask/transaction-controller` moves 69.3.0 -> 69.4.0 because
phishing-controller 17.3.1 requires ^69.4.0 for the `getEffectiveRecipient`
export. The exact `resolutions` pin has to move too, otherwise it collapses
phishing-controller back onto 69.3.0, where that export does not exist.
@adonesky1
adonesky1 force-pushed the bump/phishing-controller-17.3.1 branch from 742d8fa to 50cde74 Compare July 31, 2026 20:38
imblue-dabadee
imblue-dabadee previously approved these changes Jul 31, 2026
…ler-17.3.1

# Conflicts:
#	package.json
#	yarn.lock
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeAccounts, SmokeConfirmations, SmokeNetworkAbstractions, SmokeNetworkExpansion, SmokeSwap, SmokeStake, SmokeWalletPlatform, SmokeMoney, SmokePerps, SmokeMultiChainAPI, SmokePredictions, SmokeSeedlessOnboarding, SmokeBrowser, SmokeSnaps, SmokeMMConnect
  • Selected Performance tags: @PerformanceSwaps, @PerformanceAssetLoading, @PerformanceLogin, @PerformanceLaunch
  • Risk Level: high
  • AI Confidence: 100%
click to see 🤖 AI reasoning details

E2E Test Selection:
Hard rule (controller-version-update): @MetaMask controller package version updated in package.json: @metamask/transaction-controller, @metamask/phishing-controller. Running all tests.

Performance Test Selection:
The transaction-controller bump (with network-controller minor and remote-feature-flag-controller major bumps) could affect performance in several areas: (1) @PerformanceSwaps - TransactionController is core to swap execution, any latency changes in transaction processing would show here; (2) @PerformanceAssetLoading - NetworkController changes could affect how quickly assets/balances load across networks; (3) @PerformanceLogin - RemoteFeatureFlagController major bump (^4→^5) is initialized in Phase 1 and could affect app startup/unlock time; (4) @PerformanceLaunch - RemoteFeatureFlagController is initialized early in the app lifecycle, a major version bump could affect cold start performance.

View GitHub Actions results

@sonarqubecloud

sonarqubecloud Bot commented Aug 3, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ Performance Test Results

ℹ️ Performance test results are currently non-blocking and will not block this PR.

❌ 1 test failed · 5 tests · 1 device

📱 Devices tested (1)

Android: Google Pixel 8 Pro (v14.0)

❌ Failed Tests (1)

@metamask-mobile-platform

Measure Cold Start To Onboarding Screen

Platform Device Reason Recording
Android Google Pixel 8 Pro (v14.0) Test error 📹 Watch
✅ Passed Tests (4)
Test Platform Device Duration Team Recording
Asset View, SRP 1 + SRP 2 + SRP 3 Android Google Pixel 8 Pro (v14.0) 4.65s @assets-dev-team 📹 Watch
Cold Start: Measure ColdStart To Login Screen Android Google Pixel 8 Pro (v14.0) 4.45s @metamask-mobile-platform 📹 Watch
Measure Warm Start: Warm Start to Login Screen Android Google Pixel 8 Pro (v14.0) 0.69s @metamask-mobile-platform 📹 Watch
Measure Warm Start: Login To Wallet Screen Android Google Pixel 8 Pro (v14.0) 2.39s @metamask-mobile-platform 📹 Watch

Branch: bump/phishing-controller-17.3.1 · Build: E2E · Commit: a07ab25 · View full run

@adonesky1
adonesky1 enabled auto-merge August 3, 2026 19:40
@adonesky1
adonesky1 added this pull request to the merge queue Aug 3, 2026
Merged via the queue into main with commit a05fe09 Aug 3, 2026
252 of 255 checks passed
@adonesky1
adonesky1 deleted the bump/phishing-controller-17.3.1 branch August 3, 2026 21:22
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 3, 2026
@metamask-ci metamask-ci Bot added the release-8.7.0 Issue or pull request that will be included in release 8.7.0 label Aug 3, 2026

This branch was previously deployed

1 inactive deployment
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-8.7.0 Issue or pull request that will be included in release 8.7.0 risk:high AI analysis: high risk size-XS team-wallet-integrations Wallet Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants