diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index 5b468c563c..08c2f98b5f 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/keyring-controller` from `^27.1.0` to `^27.1.1` ([#9791](https://github.com/MetaMask/core/pull/9791)) - Bump `@metamask/network-enablement-controller` from `^6.0.2` to `^6.0.3` ([#9791](https://github.com/MetaMask/core/pull/9791)) +### Fixed + +- Stop publishing `assetsInfo` from websocket balance updates; metadata is now resolved from the Token API by `TokenDataSource` to prevent WS poisoning (incorrect WS symbols and no detection metadata). Prevents bypassing token detection spam filtering ([#9790](https://github.com/MetaMask/core/pull/9790)) + ## [13.1.1] ### Changed diff --git a/packages/assets-controller/src/AssetsController.ts b/packages/assets-controller/src/AssetsController.ts index 7b4dd19913..ed7fa8b9fc 100644 --- a/packages/assets-controller/src/AssetsController.ts +++ b/packages/assets-controller/src/AssetsController.ts @@ -896,8 +896,6 @@ export class AssetsController extends BaseController< this.#accountActivityDataSource = new AccountActivityDataSource({ messenger: this.messenger, onActiveChainsUpdated: this.#onActiveChainsUpdated, - getAssetType: (assetId: Caip19AssetId): 'native' | 'erc20' | 'spl' => - this.#getAssetType(assetId), onAssetsUpdate: (response, request): Promise => this.handleAssetsUpdate( response, diff --git a/packages/assets-controller/src/data-sources/AccountActivityDataSource.test.ts b/packages/assets-controller/src/data-sources/AccountActivityDataSource.test.ts index 2d522e7f68..4969debf0c 100644 --- a/packages/assets-controller/src/data-sources/AccountActivityDataSource.test.ts +++ b/packages/assets-controller/src/data-sources/AccountActivityDataSource.test.ts @@ -89,9 +89,8 @@ function createBalanceUpdate(overrides?: { } type SetupOptions = { - groupAccounts?: InternalAccount[]; + groupAccounts?: InternalAccount[] | (() => InternalAccount[]); selectedAccount?: InternalAccount | null; - getAssetType?: (assetId: Caip19AssetId) => 'native' | 'erc20' | 'spl'; onAssetsUpdate?: jest.Mock; onActiveChainsUpdated?: jest.Mock; state?: { activeChains?: ChainId[] }; @@ -102,7 +101,6 @@ type SetupResult = { rootMessenger: RootMessenger; onAssetsUpdate: jest.Mock; onActiveChainsUpdated: jest.Mock; - getAssetType: jest.Mock; triggerBalanceUpdated: (payload: { address: string; chain: string; @@ -154,21 +152,17 @@ function setup(options: SetupOptions = {}): SetupResult { rootMessenger.registerActionHandler( 'AccountTreeController:getAccountsFromSelectedAccountGroup', - () => groupAccounts, + () => + typeof groupAccounts === 'function' ? groupAccounts() : groupAccounts, ); rootMessenger.registerActionHandler( 'AccountsController:getSelectedAccount', () => selectedAccount as InternalAccount, ); - const getAssetType = jest - .fn() - .mockImplementation(options.getAssetType ?? ((): 'native' => 'native')); - const dataSource = new AccountActivityDataSource({ messenger: assetsControllerMessenger, onActiveChainsUpdated, - getAssetType, onAssetsUpdate, state, }); @@ -199,7 +193,6 @@ function setup(options: SetupOptions = {}): SetupResult { rootMessenger, onAssetsUpdate, onActiveChainsUpdated, - getAssetType, triggerBalanceUpdated, triggerStatusChanged, cleanup, @@ -291,14 +284,6 @@ describe('AccountActivityDataSource', () => { [ETH_ASSET]: { amount: '1' }, }, }, - assetsInfo: { - [ETH_ASSET]: { - type: 'native', - symbol: 'ETH', - name: 'ETH', - decimals: 18, - }, - }, }); expect(request).toStrictEqual({ accountsWithSupportedChains: [ @@ -311,7 +296,7 @@ describe('AccountActivityDataSource', () => { cleanup(); }); - it('converts a hex postBalance to a human-readable amount', async () => { + it('does not publish asset metadata from the websocket payload', async () => { const account = createMockAccount(); const { onAssetsUpdate, triggerBalanceUpdated, cleanup } = setup({ groupAccounts: [account], @@ -321,7 +306,12 @@ describe('AccountActivityDataSource', () => { address: EVM_ADDRESS, chain: CHAIN_MAINNET, updates: [ - createBalanceUpdate({ postBalance: { amount: '0x10aa6d94e80' } }), + createBalanceUpdate({ + // On-chain symbol/name can be attacker-controlled (e.g. scam + // URLs), so it must never reach state; the Token API is the + // metadata source of truth. + asset: { unit: 'www.scam-url.example' }, + }), ], }); @@ -329,27 +319,32 @@ describe('AccountActivityDataSource', () => { expect(onAssetsUpdate).toHaveBeenCalledTimes(1); const [response] = onAssetsUpdate.mock.calls[0]; - expect(response.assetsBalance[account.id][ETH_ASSET]).toStrictEqual({ - amount: '0.00000114526056', - }); + expect(response.assetsInfo).toBeUndefined(); cleanup(); }); - it('resolves the asset type via the injected getAssetType', async () => { - const { getAssetType, triggerBalanceUpdated, cleanup } = setup({ - getAssetType: () => 'erc20', + it('converts a hex postBalance to a human-readable amount', async () => { + const account = createMockAccount(); + const { onAssetsUpdate, triggerBalanceUpdated, cleanup } = setup({ + groupAccounts: [account], }); triggerBalanceUpdated({ address: EVM_ADDRESS, chain: CHAIN_MAINNET, - updates: [createBalanceUpdate()], + updates: [ + createBalanceUpdate({ postBalance: { amount: '0x10aa6d94e80' } }), + ], }); await Promise.resolve(); - expect(getAssetType).toHaveBeenCalledWith(ETH_ASSET); + expect(onAssetsUpdate).toHaveBeenCalledTimes(1); + const [response] = onAssetsUpdate.mock.calls[0]; + expect(response.assetsBalance[account.id][ETH_ASSET]).toStrictEqual({ + amount: '0.00000114526056', + }); cleanup(); }); @@ -546,7 +541,7 @@ describe('AccountActivityDataSource', () => { it('swallows synchronous errors thrown while handling the event', async () => { const { onAssetsUpdate, triggerBalanceUpdated, cleanup } = setup({ - getAssetType: () => { + groupAccounts: () => { throw new Error('boom'); }, }); @@ -771,7 +766,6 @@ describe('AccountActivityDataSource', () => { const dataSource = createAccountActivityDataSource({ messenger: assetsControllerMessenger, onActiveChainsUpdated: jest.fn(), - getAssetType: () => 'native', onAssetsUpdate: jest.fn(), }); diff --git a/packages/assets-controller/src/data-sources/AccountActivityDataSource.ts b/packages/assets-controller/src/data-sources/AccountActivityDataSource.ts index 13b701d80f..b6c9ffa15c 100644 --- a/packages/assets-controller/src/data-sources/AccountActivityDataSource.ts +++ b/packages/assets-controller/src/data-sources/AccountActivityDataSource.ts @@ -11,7 +11,6 @@ import type { AssetsControllerMessenger } from '../AssetsController.js'; import { projectLogger, createModuleLogger } from '../logger.js'; import type { AssetBalance, - AssetMetadata, ChainId, Caip19AssetId, DataRequest, @@ -36,15 +35,23 @@ const log = createModuleLogger(projectLogger, CONTROLLER_NAME); * Convert AccountActivityMessage balance updates into a {@link DataResponse} * for AssetsController. * + * Only balances are published. The websocket payload's asset `unit` echoes the + * on-chain contract symbol, which is attacker-controlled for airdropped tokens + * (e.g. scam-URL token names). Publishing it as metadata poisoned + * `state.assetsInfo`, marking spam assets as "known" and exempting them from + * TokenDataSource's spam filtering on all subsequent updates. Metadata is + * intentionally left for TokenDataSource to resolve from the Token API — the + * metadata source of truth — during the same pipeline pass. The payload's + * `decimals` is still used locally to convert raw amounts to human-readable + * balances. + * * @param updates - Balance updates from account-activity websocket payload. * @param accountId - Internal account UUID. - * @param getAssetType - Resolver for asset metadata type. * @returns DataResponse with merge mode when balances are present. */ function processAccountActivityBalanceUpdates( updates: BalanceUpdate[], accountId: string, - getAssetType: (assetId: Caip19AssetId) => 'native' | 'erc20' | 'spl', ): DataResponse { const assetsBalance = Object.create(null) as Record< string, @@ -54,10 +61,6 @@ function processAccountActivityBalanceUpdates( Caip19AssetId, AssetBalance >; - const assetsMetadata = Object.create(null) as Record< - Caip19AssetId, - AssetMetadata - >; for (const update of updates) { const { asset, postBalance } = update; @@ -83,19 +86,11 @@ function processAccountActivityBalanceUpdates( assetsBalance[accountId][assetId] = { amount: humanReadableAmount, }; - - assetsMetadata[assetId] = { - type: getAssetType(assetId), - symbol: asset.unit, - name: asset.unit, - decimals: asset.decimals, - }; } const response: DataResponse = { updateMode: 'merge' }; if (Object.keys(assetsBalance[accountId]).length > 0) { response.assetsBalance = assetsBalance; - response.assetsInfo = assetsMetadata; } return response; @@ -133,8 +128,6 @@ export type AccountActivityDataSourceOptions = { chains: ChainId[], previousChains: ChainId[], ) => void; - /** Returns the asset type ('native' | 'erc20' | 'spl') for a given CAIP-19 asset ID. */ - getAssetType: (assetId: Caip19AssetId) => 'native' | 'erc20' | 'spl'; /** * Pushes decoded balance updates to the controller (bound to * `AssetsController.handleAssetsUpdate`). AADS is event-driven and never @@ -196,10 +189,6 @@ export class AccountActivityDataSource extends AbstractDataSource< previousChains: ChainId[], ) => void; - readonly #getAssetType: ( - assetId: Caip19AssetId, - ) => 'native' | 'erc20' | 'spl'; - readonly #onAssetsUpdate: ( response: DataResponse, request?: DataRequest, @@ -219,7 +208,6 @@ export class AccountActivityDataSource extends AbstractDataSource< this.#messenger = options.messenger; this.#onActiveChainsUpdated = options.onActiveChainsUpdated; - this.#getAssetType = options.getAssetType; this.#onAssetsUpdate = options.onAssetsUpdate; this.#onBalanceUpdatedBound = this.#onBalanceUpdated.bind(this); @@ -285,7 +273,6 @@ export class AccountActivityDataSource extends AbstractDataSource< const response = processAccountActivityBalanceUpdates( updates, account.id, - (assetId) => this.#getAssetType(assetId), ); if (!response.assetsBalance) {