fix(assets-controller): unlock spam sweep no longer deletes mUSD or unjudged assets - #10066
Open
gomesalexandre wants to merge 3 commits into
Open
fix(assets-controller): unlock spam sweep no longer deletes mUSD or unjudged assets#10066gomesalexandre wants to merge 3 commits into
gomesalexandre wants to merge 3 commits into
Conversation
…njudged assets The unlock-time spam-cleanup sweep (healAssetsInfoMetadata.ts) exempted default-tracked assets by a 3-chain seeding registry (DEFAULT_TRACKED_ASSETS_BY_CHAIN) instead of by address, so mUSD's real, below-floor occurrence count on every OTHER Accounts-API-covered chain (e.g. BNB Smart Chain) made the sweep delete the user's holding. The sibling TokenDataSource.ts already exempts mUSD chain-agnostically by address; hoist that check into a shared isMusdAssetId() helper in defaults.ts and use it in both places. Separately, findBelowFloorAssetIds() treated an asset entirely absent from the Token API's /v3/assets response array the same as a confirmed zero occurrence count, deleting it. Distinguish 'the API never indexed this asset' (unjudgeable, keep) from 'the API described it with no occurrence count' (judge as before, preserving existing behavior for e.g. Monad tokens) -- matching the sibling occurrenceFilterMiddleware's existing contract.
gomesalexandre
marked this pull request as ready for review
September 1, 2026 23:27
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.
What it says on the box
The unlock-time spam-cleanup sweep (
healAssetsInfoMetadata.ts'scleanSpamAssets, gated behindassetsUnifyState.useUnlockCleanup) deletes MetaMask's own stablecoin, mUSD, from persisted holdings on any Accounts-API-covered chain outside a 3-chain seeding registry. Separately, it treats an asset the Token API has never heard of as a confirmed-spam zero, when it should be unjudgeable.Destructive-data-path bug — please read the honest scoping section before triaging severity.
Bug 1 — mUSD deleted on 8 of 12 chains it's actually deployed to
collectSpamCleanupCandidatesexempts assets by checking membership in[...DEFAULT_TRACKED_ASSETS_BY_CHAIN.values()]— a seeding registry meant to pre-populate default tracked assets on a handful of chains (Ethereum, Linea, Monad for mUSD; Arc for its native USDC), not an exhaustive list of every chain mUSD is deployed to. mUSD's real, low aggregator count on every other Accounts-API chain falls below the occurrence floor, and the sweep deletes it.The sibling
TokenDataSource.tsalready exempts mUSD correctly — chain-agnostically, by address — at two call sites (occurrenceFilterMiddleware,assetsMiddleware). This PR hoists that check into a sharedisMusdAssetId()helper indefaults.tsand routes bothTokenDataSource.tsand the sweep through it, so the two can't drift apart again.Verified live against the real Token API and a real chain, not just source-reading:
Bug 2 — an asset entirely absent from the Token API's response is treated as confirmed spam
findBelowFloorAssetIdsbuilds aMapfrom the API's response array and does(occurrencesByLowerId.get(assetId.toLowerCase()) ?? 0) < floorfor every requested asset — including ones the API never mentioned in its response at all (e.g. a chain it doesn't serve for that asset). That conflates "the API confirms zero occurrences" with "the API has no data," deleting a real holding on the strength of a lookup miss.The sibling
TokenDataSource.occurrenceFilterMiddlewarealready gets this right — it only iterates assets present in the response array, so one absent entirely is implicitly kept (never added tospamAssetIds). This PR gives the sweep the same distinction: an asset entirely absent from the response array is unjudgeable and kept; an asset the API did describe but scored with no occurrence count is still judged as before (preserving existing accepted behavior — every Monad token currently comes back withoccurrences: undefinedand is still correctly flagged as spam by both this sweep and its sibling).Verified against the real production API that this path is only reachable via genuine API non-coverage, not via "the API doesn't recognize an address" — critical for ruling out a false-negative regression:
Even a completely unrecognized junk address comes back as a stub entry (
occurrencesabsent, but present in the array) — it does not get omitted. So a genuine spam token can never silently exploit the "absent from response" path in production; that path only fires for the scenario it's meant to protect (a chain the API genuinely doesn't serve).Testing
Full package suite: 982/982 passing, zero regressions. Genuine red-before/green-after verified via
git stashon both the mUSD fix and the missing-vs-absent fix independently — each new/changed test fails with the exact predicted symptom against unmodified source and passes against the fix.Every pre-existing "this is spam, drop it" test case (a synthetic never-indexed token, a real token the API describes with no occurrence count, a token with a real low occurrence count) still correctly gets dropped — only the two false-positive shapes (mUSD anywhere, and an asset genuinely absent from the response array) change.
yarn lint:tsc(repo-wide typecheck): clean, exit 0.yarn eslinton all changed files: clean, exit 0.Honest scoping
assetsUnifyState.useUnlockCleanupis a server-controlled sub-flag, disabled unless explicitly enabled — I cannot observe the actual rollout percentage from source. The code has shipped in published versions; whether it has executed against real user wallets depends entirely on that rollout.KeyringController:unlock. The other consumer of the mUSD exemption (the balance-update middleware) already correctly exempts mUSD, so a subsequent successful poll after an unlock-time deletion would likely re-add the holding. The accurate claim is "mUSD vanishes at every unlock and only returns if/when the next poll succeeds" — not permanent, irrecoverable loss.Adversarial review
Ran Codex synchronously as a second reviewer; it stalled with zero output for 6+ minutes, so I killed my own tracked process (not a broad pattern kill) and did a thorough self-review instead, covering: (1) whether genuine spam could now survive — verified no, via the live-API stub-behavior check above and the preserved test cases; (2) whether the
isMusdAssetIdsubstring match could false-positive — no, it's the identical check the code already did, just centralized; (3) circular import risk from the newdefaults.ts→TokenDataSource.tsdependency — none, confirmed by a clean repo-wide typecheck; (4) whether the test changes are genuinely red-before/green-after — confirmed viagit stash, not tautological; (5) interaction between the two fixes — none, they're independent code paths (mUSD is filtered out before the occurrence check ever runs on it).Note
High Risk
Changes destructive unlock-time cleanup that deletes persisted asset holdings; incorrect logic could hide real tokens, though the fix narrows false positives and is gated behind
useUnlockCleanup.Overview
Fixes two false positives in the unlock-time spam cleanup (
cleanSpamAssets/useUnlockCleanup) that could remove legitimate holdings from persistedassetsInfoandassetsBalance.mUSD on chains outside default seeding: The sweep used
DEFAULT_TRACKED_ASSETS_BY_CHAINas an exemption list, but that map only seeds mUSD on a few chains—not every deployment. mUSD elsewhere could be classified as below the Token API occurrence floor and deleted. The PR adds sharedisMusdAssetId()indefaults.ts, wiresTokenDataSourcespam paths through it, and exempts mUSD by contract address incollectSpamCleanupCandidatesso behavior matches the live pipeline.Missing Token API rows:
findBelowFloorAssetIdstreated assets omitted from the/v3/assetsresponse as zero occurrences and removed them. It now only judges assets that appear in the response; fully absent IDs are kept as unjudgeable (aligned withTokenDataSourceoccurrence filtering, but critical here because this path deletes state).Tests and fixtures add BNB-chain mUSD and flip the “omitted from response” case from drop to keep.
Reviewed by Cursor Bugbot for commit c7bddce. Bugbot is set up for automated code reviews on this repo. Configure here.