Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/controllers/phishing/phishing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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'
)
Expand Down
32 changes: 23 additions & 9 deletions src/controllers/phishing/phishing.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { getDomain } from 'tldts'

import { zeroAddress } from 'viem'

import { RecurringTimeout } from '../../classes/recurringTimeout/recurringTimeout'
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
})

Expand Down Expand Up @@ -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,
Expand Down
22 changes: 21 additions & 1 deletion src/controllers/transfer/transfer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}

Expand Down Expand Up @@ -572,7 +580,8 @@ export class TransferController extends EventEmitter implements ITransferControl
this.isRecipientAddressFirstTimeSend,
this.lastSentToRecipientAt,
this.addressPoisoningMatch,
this.recipientDomainAddressChange
this.recipientDomainAddressChange,
this.isRecipientAddressBlacklisted
)
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
}
Expand Down
33 changes: 32 additions & 1 deletion src/services/validations/validate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}
Expand All @@ -18,6 +20,7 @@ const validate = (overrides: {
isRecipientAddressFirstTimeSend?: boolean
isRecipientAddressUnknown?: boolean
isDomain?: boolean
isRecipientAddressBlacklisted?: boolean
}) =>
validateSendTransferAddress(
RECIPIENT,
Expand All @@ -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', () => {
Expand All @@ -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)
})
})
15 changes: 13 additions & 2 deletions src/services/validations/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down