From afb578886134663506320e7462935d3431512a9a Mon Sep 17 00:00:00 2001 From: Csaba Solya Date: Wed, 30 May 2018 15:53:18 +0200 Subject: [PATCH 1/5] initial implementation --- app/scripts/controllers/transactions/index.js | 6 +++- .../lib/recipient-blacklist-checker.js | 36 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index aff5db984..3d6f5beb5 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -10,6 +10,7 @@ const NonceTracker = require('./nonce-tracker') const txUtils = require('./lib/util') const cleanErrorStack = require('../../lib/cleanErrorStack') const log = require('loglevel') +const recipientBlackListChecker = require('./lib/recipient-blacklist-checker') /** Transaction Controller is an aggregate of sub-controllers and trackers @@ -157,8 +158,11 @@ class TransactionController extends EventEmitter { let txMeta = this.txStateManager.generateTxMeta({ txParams: normalizedTxParams }) this.addTx(txMeta) this.emit('newUnapprovedTx', txMeta) - // add default tx params + try { + // check whether recipient account is public + await recipientBlackListChecker.checkAccount(txMeta.metamaskNetworkId, normalizedTxParams.to) + // add default tx params txMeta = await this.addTxGasDefaults(txMeta) } catch (error) { console.log(error) diff --git a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js new file mode 100644 index 000000000..f6fbee678 --- /dev/null +++ b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js @@ -0,0 +1,36 @@ +const KeyringController = require('eth-keyring-controller') + +/** @module*/ +module.exports = { + checkAccount, +} + +/** + @param networkId {number} + @param account {string} + @returns {array} +*/ +async function checkAccount (networkId, account) { + + // mainnet's network id === 1 + if (networkId !== 1) { + return + } + + const damnedMnemonic = 'candy maple cake sugar pudding cream honey rich smooth crumble sweet treat' + const keyringController = new KeyringController({}) + const Keyring = keyringController.getKeyringClassForType('HD Key Tree') + const opts = { + mnemonic: damnedMnemonic, + numberOfAccounts: 10, + } + + const accountToCheck = account.toLowerCase() + const keyring = new Keyring(opts) + const damnedAccounts = await keyring.getAccounts() + for (let i = 0; i < damnedAccounts.length; i++) { + if (damnedAccounts[i].toLowerCase() === accountToCheck) { + throw new Error('this is a public account') + } + } +} \ No newline at end of file From 6affd8f9492e04cdc81007e4f5390e4faa56499d Mon Sep 17 00:00:00 2001 From: Csaba Solya Date: Wed, 30 May 2018 16:24:40 +0200 Subject: [PATCH 2/5] adding transaction controller tests --- .../lib/recipient-blacklist-checker.js | 2 +- .../transactions/tx-controller-test.js | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js index f6fbee678..c52e58863 100644 --- a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js +++ b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js @@ -30,7 +30,7 @@ async function checkAccount (networkId, account) { const damnedAccounts = await keyring.getAccounts() for (let i = 0; i < damnedAccounts.length; i++) { if (damnedAccounts[i].toLowerCase() === accountToCheck) { - throw new Error('this is a public account') + throw new Error('Recipient is a public account') } } } \ No newline at end of file diff --git a/test/unit/app/controllers/transactions/tx-controller-test.js b/test/unit/app/controllers/transactions/tx-controller-test.js index 1f32a0f37..9bdfe7c1a 100644 --- a/test/unit/app/controllers/transactions/tx-controller-test.js +++ b/test/unit/app/controllers/transactions/tx-controller-test.js @@ -185,6 +185,23 @@ describe('Transaction Controller', function () { .catch(done) }) + it('should fail if recipient is public', function (done) { + txController.networkStore = new ObservableStore(1) + txController.addUnapprovedTransaction({ from: '0x1678a085c290ebd122dc42cba69373b5953b831d', to: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2' }) + .catch((err) => { + if (err.message === 'Recipient is a public account') done() + else done(err) + }) + }) + + it('should not fail if recipient is public but not on mainnet', function (done) { + txController.once('newUnapprovedTx', (txMetaFromEmit) => { + assert(txMetaFromEmit, 'txMeta is falsey') + done() + }) + txController.addUnapprovedTransaction({ from: '0x1678a085c290ebd122dc42cba69373b5953b831d', to: '0x0d1d4e623D10F9FBA5Db95830F7d3839406C6AF2' }) + .catch(done) + }) }) describe('#addTxGasDefaults', function () { From cf73581c0e1a90371fb23eb05318ce39027325b5 Mon Sep 17 00:00:00 2001 From: Csaba Solya Date: Wed, 30 May 2018 17:38:27 +0200 Subject: [PATCH 3/5] adding tests for recipient blacklist checker --- app/scripts/controllers/transactions/index.js | 4 +- .../lib/recipient-blacklist-checker.js | 6 +- .../recipient-blacklist-checker-test.js | 78 +++++++++++++++++++ 3 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 3d6f5beb5..16f7291d6 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -10,7 +10,7 @@ const NonceTracker = require('./nonce-tracker') const txUtils = require('./lib/util') const cleanErrorStack = require('../../lib/cleanErrorStack') const log = require('loglevel') -const recipientBlackListChecker = require('./lib/recipient-blacklist-checker') +const recipientBlacklistChecker = require('./lib/recipient-blacklist-checker') /** Transaction Controller is an aggregate of sub-controllers and trackers @@ -161,7 +161,7 @@ class TransactionController extends EventEmitter { try { // check whether recipient account is public - await recipientBlackListChecker.checkAccount(txMeta.metamaskNetworkId, normalizedTxParams.to) + await recipientBlacklistChecker.checkAccount(txMeta.metamaskNetworkId, normalizedTxParams.to) // add default tx params txMeta = await this.addTxGasDefaults(txMeta) } catch (error) { diff --git a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js index c52e58863..414302d12 100644 --- a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js +++ b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js @@ -12,8 +12,8 @@ module.exports = { */ async function checkAccount (networkId, account) { - // mainnet's network id === 1 - if (networkId !== 1) { + const mainnetId = 1 + if (networkId !== mainnetId) { return } @@ -33,4 +33,4 @@ async function checkAccount (networkId, account) { throw new Error('Recipient is a public account') } } -} \ No newline at end of file +} diff --git a/test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js b/test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js new file mode 100644 index 000000000..b55894684 --- /dev/null +++ b/test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js @@ -0,0 +1,78 @@ +const assert = require('assert') +const recipientBlackListChecker = require('../../../../../app/scripts/controllers/transactions/lib/recipient-blacklist-checker') +const { + ROPSTEN_CODE, + RINKEYBY_CODE, + KOVAN_CODE, +} = require('../../../../../app/scripts/controllers/network/enums') + +const KeyringController = require('eth-keyring-controller') + +describe('Recipient Blacklist Checker', function () { + + let publicAccounts + + before(async function () { + const damnedMnemonic = 'candy maple cake sugar pudding cream honey rich smooth crumble sweet treat' + const keyringController = new KeyringController({}) + const Keyring = keyringController.getKeyringClassForType('HD Key Tree') + const opts = { + mnemonic: damnedMnemonic, + numberOfAccounts: 10, + } + const keyring = new Keyring(opts) + publicAccounts = await keyring.getAccounts() + }) + + describe('#checkAccount', function () { + it('does not fail on test networks', async function () { + let callCount = 0 + const networks = [ROPSTEN_CODE, RINKEYBY_CODE, KOVAN_CODE] + for (let networkId in networks) { + await Promise.all(publicAccounts.map(async (account) => { + await recipientBlackListChecker.checkAccount(networkId, account) + callCount++ + }) + ) + } + assert.equal(callCount, 30) + }) + + it('fails on mainnet', async function () { + const mainnetId = 1 + let callCount = 0 + await Promise.all(publicAccounts.map(async (account) => { + try { + await recipientBlackListChecker.checkAccount(mainnetId, account) + assert.fail('function should have thrown an error') + } catch (err) { + assert.equal(err.message, 'Recipient is a public account') + } + callCount++ + })) + assert.equal(callCount, 10) + }) + + it('fails for public account - uppercase', async function () { + const mainnetId = 1 + const publicAccount = '0X0D1D4E623D10F9FBA5DB95830F7D3839406C6AF2' + try { + await recipientBlackListChecker.checkAccount(mainnetId, publicAccount) + assert.fail('function should have thrown an error') + } catch (err) { + assert.equal(err.message, 'Recipient is a public account') + } + }) + + it('fails for public account - lowercase', async function () { + const mainnetId = 1 + const publicAccount = '0x0d1d4e623d10f9fba5db95830f7d3839406c6af2' + try { + await recipientBlackListChecker.checkAccount(mainnetId, publicAccount) + assert.fail('function should have thrown an error') + } catch (err) { + assert.equal(err.message, 'Recipient is a public account') + } + }) + }) +}) From 3e489ea16506569950c10fc3636071075b2495e8 Mon Sep 17 00:00:00 2001 From: Csaba Solya Date: Wed, 30 May 2018 17:42:41 +0200 Subject: [PATCH 4/5] fix documentation --- .../controllers/transactions/lib/recipient-blacklist-checker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js index 414302d12..d44c1ddc1 100644 --- a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js +++ b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js @@ -6,9 +6,9 @@ module.exports = { } /** + * Checks if a specified account on a specified network is blacklisted. @param networkId {number} @param account {string} - @returns {array} */ async function checkAccount (networkId, account) { From 1dda0c646940179bec6e886117a8ecf3f0f7ab48 Mon Sep 17 00:00:00 2001 From: Csaba Solya Date: Wed, 30 May 2018 21:15:59 +0200 Subject: [PATCH 5/5] remove generating blocked accounts and use a config file instead --- app/scripts/controllers/transactions/index.js | 4 ++-- .../lib/recipient-blacklist-checker.js | 20 ++++-------------- .../lib/recipient-blacklist-config.json | 14 +++++++++++++ .../recipient-blacklist-checker-test.js | 21 +++++++++---------- 4 files changed, 30 insertions(+), 29 deletions(-) create mode 100644 app/scripts/controllers/transactions/lib/recipient-blacklist-config.json diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 16f7291d6..b53947e27 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -160,8 +160,8 @@ class TransactionController extends EventEmitter { this.emit('newUnapprovedTx', txMeta) try { - // check whether recipient account is public - await recipientBlacklistChecker.checkAccount(txMeta.metamaskNetworkId, normalizedTxParams.to) + // check whether recipient account is blacklisted + recipientBlacklistChecker.checkAccount(txMeta.metamaskNetworkId, normalizedTxParams.to) // add default tx params txMeta = await this.addTxGasDefaults(txMeta) } catch (error) { diff --git a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js index d44c1ddc1..84c6df1f0 100644 --- a/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js +++ b/app/scripts/controllers/transactions/lib/recipient-blacklist-checker.js @@ -1,4 +1,4 @@ -const KeyringController = require('eth-keyring-controller') +const Config = require('./recipient-blacklist-config.json') /** @module*/ module.exports = { @@ -10,27 +10,15 @@ module.exports = { @param networkId {number} @param account {string} */ -async function checkAccount (networkId, account) { +function checkAccount (networkId, account) { const mainnetId = 1 if (networkId !== mainnetId) { return } - const damnedMnemonic = 'candy maple cake sugar pudding cream honey rich smooth crumble sweet treat' - const keyringController = new KeyringController({}) - const Keyring = keyringController.getKeyringClassForType('HD Key Tree') - const opts = { - mnemonic: damnedMnemonic, - numberOfAccounts: 10, - } - const accountToCheck = account.toLowerCase() - const keyring = new Keyring(opts) - const damnedAccounts = await keyring.getAccounts() - for (let i = 0; i < damnedAccounts.length; i++) { - if (damnedAccounts[i].toLowerCase() === accountToCheck) { - throw new Error('Recipient is a public account') - } + if (Config.blacklist.includes(accountToCheck)) { + throw new Error('Recipient is a public account') } } diff --git a/app/scripts/controllers/transactions/lib/recipient-blacklist-config.json b/app/scripts/controllers/transactions/lib/recipient-blacklist-config.json new file mode 100644 index 000000000..b348eb72e --- /dev/null +++ b/app/scripts/controllers/transactions/lib/recipient-blacklist-config.json @@ -0,0 +1,14 @@ +{ + "blacklist": [ + "0x627306090abab3a6e1400e9345bc60c78a8bef57", + "0xf17f52151ebef6c7334fad080c5704d77216b732", + "0xc5fdf4076b8f3a5357c5e395ab970b5b54098fef", + "0x821aea9a577a9b44299b9c15c88cf3087f3b5544", + "0x0d1d4e623d10f9fba5db95830f7d3839406c6af2", + "0x2932b7a2355d6fecc4b5c0b6bd44cc31df247a2e", + "0x2191ef87e392377ec08e7c08eb105ef5448eced5", + "0x0f4f2ac550a1b4e2280d04c21cea7ebd822934b5", + "0x6330a553fc93768f612722bb8c2ec78ac90b3bbc", + "0x5aeda56215b167893e80b4fe645ba6d5bab767de" + ] +} diff --git a/test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js b/test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js index b55894684..56e8d50db 100644 --- a/test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js +++ b/test/unit/app/controllers/transactions/recipient-blacklist-checker-test.js @@ -25,39 +25,38 @@ describe('Recipient Blacklist Checker', function () { }) describe('#checkAccount', function () { - it('does not fail on test networks', async function () { + it('does not fail on test networks', function () { let callCount = 0 const networks = [ROPSTEN_CODE, RINKEYBY_CODE, KOVAN_CODE] for (let networkId in networks) { - await Promise.all(publicAccounts.map(async (account) => { - await recipientBlackListChecker.checkAccount(networkId, account) - callCount++ + publicAccounts.forEach((account) => { + recipientBlackListChecker.checkAccount(networkId, account) + callCount++ }) - ) } assert.equal(callCount, 30) }) - it('fails on mainnet', async function () { + it('fails on mainnet', function () { const mainnetId = 1 let callCount = 0 - await Promise.all(publicAccounts.map(async (account) => { + publicAccounts.forEach((account) => { try { - await recipientBlackListChecker.checkAccount(mainnetId, account) + recipientBlackListChecker.checkAccount(mainnetId, account) assert.fail('function should have thrown an error') } catch (err) { assert.equal(err.message, 'Recipient is a public account') } callCount++ - })) + }) assert.equal(callCount, 10) }) - it('fails for public account - uppercase', async function () { + it('fails for public account - uppercase', function () { const mainnetId = 1 const publicAccount = '0X0D1D4E623D10F9FBA5DB95830F7D3839406C6AF2' try { - await recipientBlackListChecker.checkAccount(mainnetId, publicAccount) + recipientBlackListChecker.checkAccount(mainnetId, publicAccount) assert.fail('function should have thrown an error') } catch (err) { assert.equal(err.message, 'Recipient is a public account')