From 8ff1d05df3c5d2310fd868ec7b7085d8081bdbab Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 8 Jul 2020 18:34:54 -0300 Subject: [PATCH] Remove recipient blocklist checker (#8943) It seems that this blocklist checker never worked correctly. Ever since the initial commit, it was comparing the Number `1` to the `networkId`, which is a string. Additionally, even if it did throw, the transaction continued unhindered. The user could still approve it, and there was no indication shown to the user that anything went wrong. Also some of the blocklist entries were incorrectly mixed-case, and were never hit. We can remove this for now, and re-add it later on after we rewrite the transaction controller. --- app/scripts/controllers/transactions/index.js | 2 - .../lib/recipient-blocklist-checker.js | 19 ------- .../transactions/lib/recipient-blocklist.js | 17 ------ .../recipient-blocklist-checker-test.js | 53 ------------------- .../transactions/tx-controller-test.js | 17 ------ 5 files changed, 108 deletions(-) delete mode 100644 app/scripts/controllers/transactions/lib/recipient-blocklist-checker.js delete mode 100644 app/scripts/controllers/transactions/lib/recipient-blocklist.js delete mode 100644 test/unit/app/controllers/transactions/recipient-blocklist-checker-test.js diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 3de5bce09..046925b1d 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -25,7 +25,6 @@ import NonceTracker from 'nonce-tracker' import * as txUtils from './lib/util' import cleanErrorStack from '../../lib/cleanErrorStack' import log from 'loglevel' -import { throwIfAccountIsBlocked } from './lib/recipient-blocklist-checker' import { TRANSACTION_TYPE_CANCEL, @@ -246,7 +245,6 @@ export default class TransactionController extends EventEmitter { this.emit('newUnapprovedTx', txMeta) try { - throwIfAccountIsBlocked(txMeta.metamaskNetworkId, normalizedTxParams.to) txMeta = await this.addTxGasDefaults(txMeta, getCodeResponse) } catch (error) { log.warn(error) diff --git a/app/scripts/controllers/transactions/lib/recipient-blocklist-checker.js b/app/scripts/controllers/transactions/lib/recipient-blocklist-checker.js deleted file mode 100644 index 745209d29..000000000 --- a/app/scripts/controllers/transactions/lib/recipient-blocklist-checker.js +++ /dev/null @@ -1,19 +0,0 @@ -import blocklist from './recipient-blocklist' -import { MAINNET_NETWORK_ID } from '../../network/enums' - -/** - * Checks if a specified account on a specified network is blocked - * @param {number} networkId - * @param {string} account - * @throws {Error} if the account is blocked on mainnet - */ -export function throwIfAccountIsBlocked (networkId, account) { - if (networkId !== MAINNET_NETWORK_ID) { - return - } - - const accountToCheck = account.toLowerCase() - if (blocklist.includes(accountToCheck)) { - throw new Error('Recipient is a public account') - } -} diff --git a/app/scripts/controllers/transactions/lib/recipient-blocklist.js b/app/scripts/controllers/transactions/lib/recipient-blocklist.js deleted file mode 100644 index fc7e49126..000000000 --- a/app/scripts/controllers/transactions/lib/recipient-blocklist.js +++ /dev/null @@ -1,17 +0,0 @@ -const blocklist = [ - // IDEX phisher - '0x9bcb0A9d99d815Bb87ee3191b1399b1Bcc46dc77', - // Ganache default seed phrases - '0x627306090abab3a6e1400e9345bc60c78a8bef57', - '0xf17f52151ebef6c7334fad080c5704d77216b732', - '0xc5fdf4076b8f3a5357c5e395ab970b5b54098fef', - '0x821aea9a577a9b44299b9c15c88cf3087f3b5544', - '0x0d1d4e623d10f9fba5db95830f7d3839406c6af2', - '0x2932b7a2355d6fecc4b5c0b6bd44cc31df247a2e', - '0x2191ef87e392377ec08e7c08eb105ef5448eced5', - '0x0f4f2ac550a1b4e2280d04c21cea7ebd822934b5', - '0x6330a553fc93768f612722bb8c2ec78ac90b3bbc', - '0x5aeda56215b167893e80b4fe645ba6d5bab767de', -] - -export default blocklist diff --git a/test/unit/app/controllers/transactions/recipient-blocklist-checker-test.js b/test/unit/app/controllers/transactions/recipient-blocklist-checker-test.js deleted file mode 100644 index d7cab5109..000000000 --- a/test/unit/app/controllers/transactions/recipient-blocklist-checker-test.js +++ /dev/null @@ -1,53 +0,0 @@ -import { strict as assert } from 'assert' -import { throwIfAccountIsBlocked } from '../../../../../app/scripts/controllers/transactions/lib/recipient-blocklist-checker' -import { ROPSTEN_NETWORK_ID, RINKEBY_NETWORK_ID, KOVAN_NETWORK_ID, GOERLI_NETWORK_ID } from '../../../../../app/scripts/controllers/network/enums' - -describe('Recipient Blocklist Checker', function () { - describe('#throwIfAccountIsBlocked', function () { - // Accounts from Ganache's original default seed phrase - const publicAccounts = [ - '0x627306090abab3a6e1400e9345bc60c78a8bef57', - '0xf17f52151ebef6c7334fad080c5704d77216b732', - '0xc5fdf4076b8f3a5357c5e395ab970b5b54098fef', - '0x821aea9a577a9b44299b9c15c88cf3087f3b5544', - '0x0d1d4e623d10f9fba5db95830f7d3839406c6af2', - '0x2932b7a2355d6fecc4b5c0b6bd44cc31df247a2e', - '0x2191ef87e392377ec08e7c08eb105ef5448eced5', - '0x0f4f2ac550a1b4e2280d04c21cea7ebd822934b5', - '0x6330a553fc93768f612722bb8c2ec78ac90b3bbc', - '0x5aeda56215b167893e80b4fe645ba6d5bab767de', - ] - - it('does not fail on test networks', function () { - const networks = [ROPSTEN_NETWORK_ID, RINKEBY_NETWORK_ID, KOVAN_NETWORK_ID, GOERLI_NETWORK_ID] - for (const networkId of networks) { - for (const account of publicAccounts) { - assert.doesNotThrow(() => throwIfAccountIsBlocked(networkId, account)) - } - } - }) - - it('fails on mainnet', function () { - for (const account of publicAccounts) { - assert.throws( - () => throwIfAccountIsBlocked('1', account), - { message: 'Recipient is a public account' }, - ) - } - }) - - it('fails for public account - uppercase', function () { - assert.throws( - () => throwIfAccountIsBlocked('1', '0X0D1D4E623D10F9FBA5DB95830F7D3839406C6AF2'), - { message: 'Recipient is a public account' }, - ) - }) - - it('fails for public account - lowercase', function () { - assert.throws( - () => throwIfAccountIsBlocked('1', '0x0d1d4e623d10f9fba5db95830f7d3839406c6af2'), - { message: 'Recipient is a public account' }, - ) - }) - }) -}) diff --git a/test/unit/app/controllers/transactions/tx-controller-test.js b/test/unit/app/controllers/transactions/tx-controller-test.js index e811bd208..e9d229aba 100644 --- a/test/unit/app/controllers/transactions/tx-controller-test.js +++ b/test/unit/app/controllers/transactions/tx-controller-test.js @@ -198,27 +198,10 @@ describe('Transaction Controller', function () { .catch(done) }) - it('should fail if recipient is public', async function () { - txController.networkStore = new ObservableStore('1') - await assert.rejects( - () => txController.addUnapprovedTransaction({ from: selectedAddress, to: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2' }), - { message: 'Recipient is a public account' }, - ) - }) - it("should fail if the from address isn't the selected address", async function () { await assert.rejects(() => txController.addUnapprovedTransaction({ from: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2' })) }) - it('should not fail if recipient is public but not on mainnet', function (done) { - txController.once('newUnapprovedTx', (txMetaFromEmit) => { - assert.ok(txMetaFromEmit, 'txMeta is falsy') - done() - }) - txController.addUnapprovedTransaction({ from: selectedAddress, to: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2' }) - .catch(done) - }) - it('should fail if netId is loading', async function () { txController.networkStore = new ObservableStore('loading') await assert.rejects(