Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
42 changes: 39 additions & 3 deletions src/controllers/phishing/phishing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,44 @@ 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 list and of the checked address', async () => {
const { controller: lowercaseList } = await prepareTest([], [LOWERCASE_SCAM_ADDRESS])
expect(lowercaseList.getAddressBlacklistedStatus(LOWERCASE_SCAM_ADDRESS)).toBe('BLACKLISTED')
expect(lowercaseList.getAddressBlacklistedStatus(CHECKSUMMED_SCAM_ADDRESS)).toBe(
'BLACKLISTED'
)

const { controller: checksummedList } = await prepareTest([], [CHECKSUMMED_SCAM_ADDRESS])
expect(checksummedList.getAddressBlacklistedStatus(CHECKSUMMED_SCAM_ADDRESS)).toBe(
'BLACKLISTED'
)
expect(checksummedList.getAddressBlacklistedStatus(LOWERCASE_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 +266,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: 26 additions & 6 deletions src/controllers/phishing/phishing.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { getDomain } from 'tldts'
import { zeroAddress } from 'viem'

import { getAddress, zeroAddress } from 'viem'

import { RecurringTimeout } from '../../classes/recurringTimeout/recurringTimeout'
import {
Expand Down Expand Up @@ -537,11 +538,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 +682,29 @@ 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

// The list may hold addresses in any casing, while the checked address can come straight from
// user input (typed, pasted or resolved from a name). Compare every common form, so that a
// lowercase input is never treated as safe only because the list holds it checksummed.
if (this.#addresses.has(address) || this.#addresses.has(address.toLowerCase()))
return 'BLACKLISTED'

try {
if (this.#addresses.has(getAddress(address))) return 'BLACKLISTED'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? The addresses returned by the api seem all lowercase. Can you do a quick check with a script and if that's true, lowercase them on add() to ensure that they will be so in the future, add a comment and only check the lowercase version here. I know it's not from this PR, but it's not a good practice to trust the API when storing data and not updating it often.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the backend currently returns only lower case addresses, but it was not forced by the backend, this is simply how addresses happen to be stored in the upstream github repos. I will update the phishing controller to parse addresses to be lower case always and remove the extra logic here

} catch {
// Not a valid address, so it cannot be in the list
}

return '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
34 changes: 33 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,31 @@ describe('validateSendTransferAddress - recipient domain address change', () =>
expect(result.message).not.toBe(CHANGED_MESSAGE)
})
})

describe('validateSendTransferAddress - blacklisted recipient', () => {
it('warns when the recipient is in the phishing list', () => {
const result = validate({ isRecipientAddressBlacklisted: true })

expect(result.message).toBe(BLACKLISTED_MESSAGE)
// Not 'error', because an error severity disables the buttons of the transfer form. The user is
// stopped by the hold-to-proceed step instead.
expect(result.severity).toBe('warning')
})

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 stays 'warning' because 'error' disables the buttons of the form. The user is
// stopped by the hold-to-proceed step instead, which keeps the flow the same as the signing step.
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
Loading