diff --git a/src/controllers/phishing/phishing.test.ts b/src/controllers/phishing/phishing.test.ts index 30e8310bb9..a40ac609d9 100644 --- a/src/controllers/phishing/phishing.test.ts +++ b/src/controllers/phishing/phishing.test.ts @@ -82,6 +82,34 @@ describe('PhishingController', () => { ).not.toBe('BLACKLISTED') // addresses are checked separately via updateAddressesBlacklistedStatus }) + describe('getAddressBlacklistedStatus', () => { + const LOWERCASE_SCAM_ADDRESS = '0x20a9ff01b49cd8967cdd8081c547236eed1d1a4e' + const CHECKSUMMED_SCAM_ADDRESS = '0x20A9Ff01B49cD8967Cdd8081C547236EED1D1a4e' + const SAFE_ADDRESS = '0x77777777789A8BBEE6C64381e5E89E501fb0e4c8' + + test('should return BLACKLISTED for a listed address, whatever the casing of the checked address', async () => { + const { controller } = await prepareTest([], [LOWERCASE_SCAM_ADDRESS]) + expect(controller.getAddressBlacklistedStatus(LOWERCASE_SCAM_ADDRESS)).toBe('BLACKLISTED') + expect(controller.getAddressBlacklistedStatus(CHECKSUMMED_SCAM_ADDRESS)).toBe('BLACKLISTED') + }) + + test('should return VERIFIED for an address that is not in the list', async () => { + const { controller } = await prepareTest([], [LOWERCASE_SCAM_ADDRESS]) + expect(controller.getAddressBlacklistedStatus(SAFE_ADDRESS)).toBe('VERIFIED') + }) + + test('should return VERIFIED and never throw for input that is not an address', async () => { + const { controller } = await prepareTest([], [LOWERCASE_SCAM_ADDRESS]) + expect(controller.getAddressBlacklistedStatus('not-an-address')).toBe('VERIFIED') + expect(controller.getAddressBlacklistedStatus('')).toBe('VERIFIED') + }) + + test('should return undefined while the list is empty, so that callers can tell it apart from a checked address', async () => { + const { controller } = await prepareTest() + expect(controller.getAddressBlacklistedStatus(LOWERCASE_SCAM_ADDRESS)).toBeUndefined() + }) + }) + test('should switch phishing update interval to active when an active view is added and back to inactive when all active views are closed', async () => { const { controller, ui } = await prepareTest() @@ -228,9 +256,7 @@ describe('PhishingController', () => { const { controller } = await prepareTest(['example.web.app']) expect(controller.getDomainBlacklistedStatus('https://example.web.app')).toBe('BLACKLISTED') - expect(controller.getDomainBlacklistedStatus('https://example.web.app./')).toBe( - 'BLACKLISTED' - ) + expect(controller.getDomainBlacklistedStatus('https://example.web.app./')).toBe('BLACKLISTED') expect(controller.getDomainBlacklistedStatus('https://example.web.app./claim?ref=1')).toBe( 'BLACKLISTED' ) diff --git a/src/controllers/phishing/phishing.ts b/src/controllers/phishing/phishing.ts index 8d310dd3e0..bad97ee152 100644 --- a/src/controllers/phishing/phishing.ts +++ b/src/controllers/phishing/phishing.ts @@ -1,4 +1,5 @@ import { getDomain } from 'tldts' + import { zeroAddress } from 'viem' import { RecurringTimeout } from '../../classes/recurringTimeout/recurringTimeout' @@ -285,7 +286,6 @@ export class PhishingController extends EventEmitter implements IPhishingControl this.#updatedAt = phishing.updatedAt this.#domains = new Set(phishing.domains) this.#addresses = new Set(phishing.addresses) - this.updatePhishingInterval.start({ runImmediately: true }) this.isReady = true @@ -368,15 +368,22 @@ export class PhishingController extends EventEmitter implements IPhishingControl ) ;(phishing.addresses || []).forEach( ({ op, address }: { op: 'add' | 'remove'; address: string }) => { - if (op === 'add') this.#addresses.add(address) - if (op === 'remove') this.#addresses.delete(address) + // Normalized to lowercase so getAddressBlacklistedStatus can do a plain lookup, + // regardless of the casing the relayer used. + const normalizedAddress = address.toLowerCase() + if (op === 'add') this.#addresses.add(normalizedAddress) + if (op === 'remove') this.#addresses.delete(normalizedAddress) } ) } else { // Initial/full update: replace local sets with the server snapshot. this.#version = phishing.version || 0 this.#domains = new Set(phishing.domains || []) - this.#addresses = new Set(phishing.addresses || []) + // Normalized to lowercase so getAddressBlacklistedStatus can do a plain lookup, regardless + // of the casing the relayer used. + this.#addresses = new Set( + (phishing.addresses || []).map((address: string) => address.toLowerCase()) + ) } this.#shouldSyncDapps = true @@ -537,11 +544,7 @@ export class PhishingController extends EventEmitter implements IPhishingControl }) addresses.forEach((addr) => { - const status = this.#addresses.size - ? this.#addresses.has(addr) - ? 'BLACKLISTED' - : 'VERIFIED' - : undefined + const status = this.getAddressBlacklistedStatus(addr) if (status) this.#addressesBlacklistedStatus.set(addr, status) }) @@ -685,6 +688,17 @@ export class PhishingController extends EventEmitter implements IPhishingControl return undefined } + /** + * Resolves the blacklisted status of an address from the locally stored phishing list, without a + * network request. Returns undefined while the list is not loaded yet, so that callers can tell + * "not blacklisted" apart from "not checked yet". + */ + getAddressBlacklistedStatus(address: string): BlacklistedStatus | undefined { + if (!this.#addresses.size) return undefined + + return this.#addresses.has(address.toLowerCase()) ? 'BLACKLISTED' : 'VERIFIED' + } + toJSON() { return { ...this, diff --git a/src/controllers/transfer/transfer.ts b/src/controllers/transfer/transfer.ts index 8af402be86..a6ef939d6a 100644 --- a/src/controllers/transfer/transfer.ts +++ b/src/controllers/transfer/transfer.ts @@ -283,6 +283,14 @@ export class TransferController extends EventEmitter implements ITransferControl this.propagateUpdate(forceEmit) }) + // isRecipientAddressBlacklisted reads the phishing list, which loads from storage and refreshes + // in the background, so the UI has to be told when the answer may have changed + this.#phishing.onUpdate((forceEmit) => { + if (!this.#currentTransferSessionId || !isAddress(this.recipientAddress)) return + + this.propagateUpdate(forceEmit) + }, 'transfer-recipient-phishing-check') + this.emitUpdate() } @@ -572,7 +580,8 @@ export class TransferController extends EventEmitter implements ITransferControl this.isRecipientAddressFirstTimeSend, this.lastSentToRecipientAt, this.addressPoisoningMatch, - this.recipientDomainAddressChange + this.recipientDomainAddressChange, + this.isRecipientAddressBlacklisted ) } @@ -612,6 +621,16 @@ export class TransferController extends EventEmitter implements ITransferControl return getAddressFromAddressState(this.addressState) } + /** + * Whether the recipient is in the locally stored phishing list. The list is kept up to date by + * the PhishingController, so the lookup needs no network request. + */ + get isRecipientAddressBlacklisted() { + if (!isAddress(this.recipientAddress)) return false + + return this.#phishing.getAddressBlacklistedStatus(this.recipientAddress) === 'BLACKLISTED' + } + async update({ humanizerInfo, selectedToken, @@ -1290,6 +1309,7 @@ export class TransferController extends EventEmitter implements ITransferControl shouldSkipTransactionQueuedModal: this.shouldSkipTransactionQueuedModal, hasPersistedState: this.hasPersistedState, isRecipientAddressViewOnly: this.isRecipientAddressViewOnly, + isRecipientAddressBlacklisted: this.isRecipientAddressBlacklisted, amountAdjustmentWarning: this.amountAdjustmentWarning } } diff --git a/src/services/validations/validate.test.ts b/src/services/validations/validate.test.ts index 5a1d951407..b7a2ec6e6b 100644 --- a/src/services/validations/validate.test.ts +++ b/src/services/validations/validate.test.ts @@ -8,6 +8,8 @@ const RECIPIENT = '0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045' const SELECTED_ACCOUNT = '0xf9D6794F16CDbdC5b4873AEdeF4dC69d8D5edcaD' const CHANGED_MESSAGE = 'This name now resolves to a different address than the last time you sent to it. Verify the new recipient before proceeding.' +const BLACKLISTED_MESSAGE = + 'This address is known for stealing funds. Anything you send to it will be lost.' const networks: Network[] = [] const accountStates: AccountStates = {} @@ -18,6 +20,7 @@ const validate = (overrides: { isRecipientAddressFirstTimeSend?: boolean isRecipientAddressUnknown?: boolean isDomain?: boolean + isRecipientAddressBlacklisted?: boolean }) => validateSendTransferAddress( RECIPIENT, @@ -34,7 +37,8 @@ const validate = (overrides: { overrides.isRecipientAddressFirstTimeSend ?? false, null, null, - overrides.recipientDomainAddressChange ?? null + overrides.recipientDomainAddressChange ?? null, + overrides.isRecipientAddressBlacklisted ?? false ) describe('validateSendTransferAddress - recipient domain address change', () => { @@ -61,3 +65,30 @@ describe('validateSendTransferAddress - recipient domain address change', () => expect(result.message).not.toBe(CHANGED_MESSAGE) }) }) + +describe('validateSendTransferAddress - blacklisted recipient', () => { + it('errors when the recipient is in the phishing list', () => { + const result = validate({ isRecipientAddressBlacklisted: true }) + + expect(result.message).toBe(BLACKLISTED_MESSAGE) + // 'error' keeps the buttons of the transfer form disabled, so the user cannot proceed. + expect(result.severity).toBe('error') + }) + + it('takes priority over every other recipient message', () => { + const result = validate({ + isRecipientAddressBlacklisted: true, + recipientDomainAddressChange: { previousAddress: SELECTED_ACCOUNT }, + isRecipientAddressFirstTimeSend: true, + isRecipientAddressUnknown: true + }) + + expect(result.message).toBe(BLACKLISTED_MESSAGE) + }) + + it('does not warn when the recipient is not in the phishing list', () => { + const result = validate({ isRecipientAddressBlacklisted: false }) + + expect(result.message).not.toBe(BLACKLISTED_MESSAGE) + }) +}) diff --git a/src/services/validations/validate.ts b/src/services/validations/validate.ts index eaadd518ac..00647a3acd 100644 --- a/src/services/validations/validate.ts +++ b/src/services/validations/validate.ts @@ -140,7 +140,8 @@ const validateSendTransferAddress = ( isRecipientAddressFirstTimeSend?: boolean, lastRecipientTransactionDate?: Date | null, addressPoisoningMatch?: AddressPoisoningMatch | null, - recipientDomainAddressChange?: { previousAddress: string } | null + recipientDomainAddressChange?: { previousAddress: string } | null, + isRecipientAddressBlacklisted?: boolean ): Validation => { // Basic validation is handled in the AddressInput component and we don't want to overwrite it. if (!isValidAddress(address) || isRecipientDomainResolving) { @@ -150,6 +151,16 @@ const validateSendTransferAddress = ( } } + // A known scam address is the most severe problem, so it takes priority over every other message. + // The severity is 'error' so that the buttons of the form stay disabled - sending to a known scam + // address is never something we let the user proceed with. + if (isRecipientAddressBlacklisted) { + return { + message: 'This address is known for stealing funds. Anything you send to it will be lost.', + severity: 'error' + } + } + // A domain the user sent to before now resolves to a different address - it may have expired and // been re-pointed. if (recipientDomainAddressChange) { @@ -287,7 +298,7 @@ const validateSendTransferAmount = (amount: string, selectedAsset: TokenResult): } } } - } catch (e) { + } catch { // Keep original behavior but avoid adding new console usage beyond existing // callers may log if needed; return a warning indicating invalid amount. return {