From 9c6c277b8b5d33568d81a81c3e13fc06b0f7241a Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 14:38:04 -0800 Subject: [PATCH 1/8] Add replay protection to Transaction Manager Fixes #897 Needs tests. --- app/scripts/metamask-controller.js | 6 ++--- app/scripts/transaction-manager.js | 43 +++++++++++++++++------------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5df10672a..31bb4a9c2 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -177,8 +177,8 @@ module.exports = class MetamaskController { // tx signing approveTransaction: this.newUnsignedTransaction.bind(this), signTransaction: (...args) => { - this.setupSigningListners(...args) - this.txManager.formatTxForSigining(...args) + this.setupSigningListeners(...args) + this.txManager.formatTxForSigning(...args) this.sendUpdate() }, @@ -257,7 +257,7 @@ module.exports = class MetamaskController { }) } - setupSigningListners (txParams) { + setupSigningListeners (txParams) { var txId = txParams.metamaskId // apply event listeners for signing and formating events this.txManager.once(`${txId}:formatted`, this.keyringController.signTransaction.bind(this.keyringController)) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index fd136a51b..3cf12b016 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -137,26 +137,33 @@ module.exports = class TransactionManager extends EventEmitter { } // formats txParams so the keyringController can sign it - formatTxForSigining (txParams, cb) { - var address = txParams.from - var metaTx = this.getTx(txParams.metamaskId) - var gasMultiplier = metaTx.gasMultiplier - var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) - gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) - txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) + formatTxForSigning (txParams, cb) { + this.getNetwork((err, networkId) => { + if (err) { + return cb(err) + } - // normalize values - txParams.to = normalize(txParams.to) - txParams.from = normalize(txParams.from) - txParams.value = normalize(txParams.value) - txParams.data = normalize(txParams.data) - txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) - txParams.nonce = normalize(txParams.nonce) - const ethTx = new Transaction(txParams) + var address = txParams.from + var metaTx = this.getTx(txParams.metamaskId) + var gasMultiplier = metaTx.gasMultiplier + var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) + gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) + txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) - // listener is assigned in metamaskController - this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) - } + // normalize values + txParams.to = normalize(txParams.to) + txParams.from = normalize(txParams.from) + txParams.value = normalize(txParams.value) + txParams.data = normalize(txParams.data) + txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) + txParams.nonce = normalize(txParams.nonce) + + const ethTx = new Transaction(txParams, parseInt(networkId)) + + // listener is assigned in metamaskController + this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) + }) + } // receives a signed tx object and updates the tx hash // and pass it to the cb to be sent off From d2fb48f8ba1c717d3360d48551b568b511b7d84d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 14:41:04 -0800 Subject: [PATCH 2/8] Bump changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 239203553..0a06314e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Current Master - Add a check for when a tx is included in a block. +- Implement replay attack protections allowed by EIP 155. ## 2.14.1 2016-12-20 From 7320095079717b296776ac8e38571020a5d558cf Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 14:54:31 -0800 Subject: [PATCH 3/8] Linted --- app/scripts/transaction-manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index 3cf12b016..7fcdbe7ca 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -162,8 +162,8 @@ module.exports = class TransactionManager extends EventEmitter { // listener is assigned in metamaskController this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) - }) - } + }) + } // receives a signed tx object and updates the tx hash // and pass it to the cb to be sent off From 305cda4265e84d520d21c473a796e8f71febc57f Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 15:31:05 -0800 Subject: [PATCH 4/8] Use chainId parameter instead of second argument --- app/scripts/transaction-manager.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index 7fcdbe7ca..d426993a4 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -157,8 +157,9 @@ module.exports = class TransactionManager extends EventEmitter { txParams.data = normalize(txParams.data) txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) txParams.nonce = normalize(txParams.nonce) + txParams.chainId = parseInt(networkId) - const ethTx = new Transaction(txParams, parseInt(networkId)) + const ethTx = new Transaction(txParams) // listener is assigned in metamaskController this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) From e6da8e2762cd54975c334314357f1cd27cc980c8 Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 4 Jan 2017 13:04:33 -0800 Subject: [PATCH 5/8] Fix signing of transactions --- app/scripts/keyring-controller.js | 28 +++++++++-------- app/scripts/metamask-controller.js | 30 ++++++++++++------- app/scripts/transaction-manager.js | 48 +++++++++++++++++------------- 3 files changed, 61 insertions(+), 45 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 016740d88..acec8feb1 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -310,20 +310,22 @@ module.exports = class KeyringController extends EventEmitter { // // This method signs tx and returns a promise for // TX Manager to update the state after signing - signTransaction (ethTx, selectedAddress, txId, cb) { - try { - const address = normalize(selectedAddress) - return this.getKeyringForAccount(address) - .then((keyring) => { - return keyring.signTransaction(address, ethTx) - }).then((tx) => { - this.emit(`${txId}:signed`, {tx, txId, cb}) - }) - } catch (e) { - cb(e) - } - } + signTransaction (ethTx, selectedAddress, txId) { + return new Promise((resolve, reject) => { + try { + const address = normalize(selectedAddress) + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.signTransaction(address, ethTx) + }).then((tx) => { + resolve({tx, txId}) + }) + } catch (e) { + reject(e) + } + }) + } // Add Unconfirmed Message // @object msgParams // @function cb diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5df10672a..32e9faab2 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -176,10 +176,25 @@ module.exports = class MetamaskController { }, // tx signing approveTransaction: this.newUnsignedTransaction.bind(this), - signTransaction: (...args) => { - this.setupSigningListners(...args) - this.txManager.formatTxForSigining(...args) - this.sendUpdate() + signTransaction: (txParams, cb) => { + this.txManager.formatTxForSigining(txParams) + .then(({ethTx, address, txId}) => { + return this.keyringController.signTransaction(ethTx, address, txId) + }) + .then(({tx, txId}) => { + return this.txManager.resolveSignedTransaction({tx, txId}) + }) + .then((rawTx) => { + cb(null, rawTx) + }) + .catch((err) => { + console.error(err) + cb(err) + }) + .then(() => { + this.sendUpdate() + this.txManager.emit(`${txParams.metamaskId}:signingComplete`) + }) }, // msg signing @@ -257,13 +272,6 @@ module.exports = class MetamaskController { }) } - setupSigningListners (txParams) { - var txId = txParams.metamaskId - // apply event listeners for signing and formating events - this.txManager.once(`${txId}:formatted`, this.keyringController.signTransaction.bind(this.keyringController)) - this.keyringController.once(`${txId}:signed`, this.txManager.resolveSignedTransaction.bind(this.txManager)) - } - enforceTxValidations (txParams) { if (('value' in txParams) && txParams.value.indexOf('-') === 0) { const msg = `Invalid transaction value of ${txParams.value} not a positive number.` diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index fd136a51b..40078d644 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -17,7 +17,7 @@ module.exports = class TransactionManager extends EventEmitter { this.provider = opts.provider this.blockTracker = opts.blockTracker this.txProviderUtils = new TxProviderUtil(this.provider) - this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) + // this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) this.getGasMultiplier = opts.getGasMultiplier this.getNetwork = opts.getNetwork } @@ -128,7 +128,7 @@ module.exports = class TransactionManager extends EventEmitter { approveTransaction (txId, cb = warn) { this.setTxStatusSigned(txId) - cb() + this.once(`${txId}:signingComplete`, cb) } cancelTransaction (txId, cb = warn) { @@ -137,25 +137,30 @@ module.exports = class TransactionManager extends EventEmitter { } // formats txParams so the keyringController can sign it - formatTxForSigining (txParams, cb) { - var address = txParams.from - var metaTx = this.getTx(txParams.metamaskId) - var gasMultiplier = metaTx.gasMultiplier - var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) - gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) - txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) + formatTxForSigining (txParams) { + return new Promise((resolve, reject) => { + try { + var address = txParams.from + var metaTx = this.getTx(txParams.metamaskId) + var gasMultiplier = metaTx.gasMultiplier + var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) + gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) + txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) - // normalize values - txParams.to = normalize(txParams.to) - txParams.from = normalize(txParams.from) - txParams.value = normalize(txParams.value) - txParams.data = normalize(txParams.data) - txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) - txParams.nonce = normalize(txParams.nonce) - const ethTx = new Transaction(txParams) - - // listener is assigned in metamaskController - this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) + // normalize values + txParams.to = normalize(txParams.to) + txParams.from = normalize(txParams.from) + txParams.value = normalize(txParams.value) + txParams.data = normalize(txParams.data) + txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) + txParams.nonce = normalize(txParams.nonce) + const ethTx = new Transaction(txParams) + var txId = txParams.metamaskId + resolve({ethTx, address, txId}) + } catch (err) { + reject(err) + } + }) } // receives a signed tx object and updates the tx hash @@ -167,7 +172,8 @@ module.exports = class TransactionManager extends EventEmitter { metaTx.hash = txHash this.updateTx(metaTx) var rawTx = ethUtil.bufferToHex(tx.serialize()) - cb(null, rawTx) + return Promise.resolve(rawTx) + } /* From 32e8063a778740cddbda67008bd68aa7500b9191 Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 4 Jan 2017 13:13:34 -0800 Subject: [PATCH 6/8] clean up --- app/scripts/transaction-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index 40078d644..a2dba69b5 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -17,7 +17,7 @@ module.exports = class TransactionManager extends EventEmitter { this.provider = opts.provider this.blockTracker = opts.blockTracker this.txProviderUtils = new TxProviderUtil(this.provider) - // this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) + this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) this.getGasMultiplier = opts.getGasMultiplier this.getNetwork = opts.getNetwork } From cf6817092b92930ab96e1c9e610669b485b384ff Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 4 Jan 2017 15:03:45 -0800 Subject: [PATCH 7/8] remove unnecessary try statments --- app/scripts/keyring-controller.js | 18 +++++--------- app/scripts/metamask-controller.js | 3 +-- app/scripts/transaction-manager.js | 38 +++++++++++++----------------- 3 files changed, 23 insertions(+), 36 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 1a26a887f..c58be0aae 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -318,18 +318,12 @@ module.exports = class KeyringController extends EventEmitter { // TX Manager to update the state after signing signTransaction (ethTx, selectedAddress, txId) { - return new Promise((resolve, reject) => { - try { - const address = normalize(selectedAddress) - return this.getKeyringForAccount(address) - .then((keyring) => { - return keyring.signTransaction(address, ethTx) - }).then((tx) => { - resolve({tx, txId}) - }) - } catch (e) { - reject(e) - } + const address = normalize(selectedAddress) + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.signTransaction(address, ethTx) + }).then((tx) => { + return {tx, txId} }) } // Add Unconfirmed Message diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 4caad46e1..d520a59ad 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -165,7 +165,6 @@ module.exports = class MetamaskController { sendUpdate () { this.getState() .then((state) => { - this.listeners.forEach((remote) => { remote.sendUpdate(state) }) @@ -461,7 +460,7 @@ module.exports = class MetamaskController { return this.state.network } - markAccountsFound(cb) { + markAccountsFound (cb) { this.configManager.setLostAccounts([]) this.sendUpdate() cb(null, this.getState()) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index a2dba69b5..6becfa6d1 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -138,29 +138,23 @@ module.exports = class TransactionManager extends EventEmitter { // formats txParams so the keyringController can sign it formatTxForSigining (txParams) { - return new Promise((resolve, reject) => { - try { - var address = txParams.from - var metaTx = this.getTx(txParams.metamaskId) - var gasMultiplier = metaTx.gasMultiplier - var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) - gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) - txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) + var address = txParams.from + var metaTx = this.getTx(txParams.metamaskId) + var gasMultiplier = metaTx.gasMultiplier + var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) + gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) + txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) - // normalize values - txParams.to = normalize(txParams.to) - txParams.from = normalize(txParams.from) - txParams.value = normalize(txParams.value) - txParams.data = normalize(txParams.data) - txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) - txParams.nonce = normalize(txParams.nonce) - const ethTx = new Transaction(txParams) - var txId = txParams.metamaskId - resolve({ethTx, address, txId}) - } catch (err) { - reject(err) - } - }) + // normalize values + txParams.to = normalize(txParams.to) + txParams.from = normalize(txParams.from) + txParams.value = normalize(txParams.value) + txParams.data = normalize(txParams.data) + txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) + txParams.nonce = normalize(txParams.nonce) + const ethTx = new Transaction(txParams) + var txId = txParams.metamaskId + return Promise.resolve({ethTx, address, txId}) } // receives a signed tx object and updates the tx hash From 7659f5894a76bb905529d7e5ea88a156d06373d2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 5 Jan 2017 11:03:30 -0800 Subject: [PATCH 8/8] Combine two identical then blocks --- app/scripts/metamask-controller.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index d520a59ad..02e65bf10 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -194,15 +194,13 @@ module.exports = class MetamaskController { }) .then((rawTx) => { cb(null, rawTx) + this.sendUpdate() + this.txManager.emit(`${txParams.metamaskId}:signingComplete`) }) .catch((err) => { console.error(err) cb(err) }) - .then(() => { - this.sendUpdate() - this.txManager.emit(`${txParams.metamaskId}:signingComplete`) - }) }, // msg signing