From 222e62d7f10ffe22dd606aea9c15e1547986c4ab Mon Sep 17 00:00:00 2001 From: Howard Braham Date: Mon, 17 Sep 2018 20:04:10 -0700 Subject: [PATCH 1/7] Bug Fix: #1789 and #4525 eth.getCode() with no contract --- CHANGELOG.md | 1 + app/_locales/en/messages.json | 3 +++ .../controllers/transactions/tx-gas-utils.js | 23 +++++++++++++------ old-ui/app/components/pending-tx.js | 2 +- .../confirm-transaction-base.component.js | 2 +- ui/app/components/send/send.utils.js | 2 +- ui/app/constants/error-keys.js | 1 + ui/app/helpers/transactions.util.js | 2 +- 8 files changed, 25 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aef455127..6adb6f64b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Update transaction statuses when switching networks. - [#5470](https://github.com/MetaMask/metamask-extension/pull/5470) 100% coverage in French locale, fixed the procedure to verify proposed locale. +- [#5283](https://github.com/MetaMask/metamask-extension/pull/5283): Fix bug when eth.getCode() called with no contract ## 4.12.0 Thursday September 27 2018 diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index 66f378ab8..94716a7ad 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -1154,6 +1154,9 @@ "transactionError": { "message": "Transaction Error. Exception thrown in contract code." }, + "transactionErrorNoContract": { + "message": "Trying to call a contract function at an address that is not a contract address." + }, "transactionMemo": { "message": "Transaction memo (optional)" }, diff --git a/app/scripts/controllers/transactions/tx-gas-utils.js b/app/scripts/controllers/transactions/tx-gas-utils.js index 3dd45507f..5ec728085 100644 --- a/app/scripts/controllers/transactions/tx-gas-utils.js +++ b/app/scripts/controllers/transactions/tx-gas-utils.js @@ -7,6 +7,8 @@ const { const { addHexPrefix } = require('ethereumjs-util') const SIMPLE_GAS_COST = '0x5208' // Hex for 21000, cost of a simple send. +import { TRANSACTION_NO_CONTRACT_ERROR_KEY } from '../../../../ui/app/constants/error-keys' + /** tx-gas-utils are gas utility methods for Transaction manager its passed ethquery @@ -32,6 +34,7 @@ class TxGasUtil { } catch (err) { txMeta.simulationFails = { reason: err.message, + errorKey: err.errorKey, } return txMeta } @@ -56,16 +59,22 @@ class TxGasUtil { return txParams.gas } - // if recipient has no code, gas is 21k max: const recipient = txParams.to const hasRecipient = Boolean(recipient) - let code - if (recipient) code = await this.query.getCode(recipient) - if (hasRecipient && (!code || code === '0x')) { - txParams.gas = SIMPLE_GAS_COST - txMeta.simpleSend = true // Prevents buffer addition - return SIMPLE_GAS_COST + if (hasRecipient) { + let code = await this.query.getCode(recipient) + + // If there's data in the params, but there's no code, it's not a valid contract + // For no code, Infura will return '0x', and Ganache will return '0x0' + if (txParams.data && (!code || code === '0x' || code === '0x0')) { + throw {errorKey: TRANSACTION_NO_CONTRACT_ERROR_KEY} + } + else if (!code) { + txParams.gas = SIMPLE_GAS_COST // For a standard ETH send, gas is 21k max + txMeta.simpleSend = true // Prevents buffer addition + return SIMPLE_GAS_COST + } } // if not, fall back to block gasLimit diff --git a/old-ui/app/components/pending-tx.js b/old-ui/app/components/pending-tx.js index c8132539c..7d8c94699 100644 --- a/old-ui/app/components/pending-tx.js +++ b/old-ui/app/components/pending-tx.js @@ -489,7 +489,7 @@ PendingTx.prototype.verifyGasParams = function () { } PendingTx.prototype._notZeroOrEmptyString = function (obj) { - return obj !== '' && obj !== '0x0' + return obj !== '' && obj !== '0x0' && obj !== '0x' // The '0x' case might not ever happen, but it seems safest to protect against it } PendingTx.prototype.bnMultiplyByFraction = function (targetBN, numerator, denominator) { diff --git a/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js index 707dad62d..9e6341722 100644 --- a/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -131,7 +131,7 @@ export default class ConfirmTransactionBase extends Component { if (simulationFails) { return { valid: true, - errorKey: TRANSACTION_ERROR_KEY, + errorKey: simulationFails.errorKey ? simulationFails.errorKey : TRANSACTION_ERROR_KEY, } } diff --git a/ui/app/components/send/send.utils.js b/ui/app/components/send/send.utils.js index a18a9e4b3..05ba6b88f 100644 --- a/ui/app/components/send/send.utils.js +++ b/ui/app/components/send/send.utils.js @@ -215,7 +215,7 @@ async function estimateGas ({ // if recipient has no code, gas is 21k max: if (!selectedToken && !data) { const code = Boolean(to) && await global.eth.getCode(to) - if (!code || code === '0x') { + if (!code || code === '0x' || code === '0x0') { // Infura will return '0x', and Ganache will return '0x0' return SIMPLE_GAS_COST } } else if (selectedToken && !to) { diff --git a/ui/app/constants/error-keys.js b/ui/app/constants/error-keys.js index f70ed3b19..704064c96 100644 --- a/ui/app/constants/error-keys.js +++ b/ui/app/constants/error-keys.js @@ -1,3 +1,4 @@ export const INSUFFICIENT_FUNDS_ERROR_KEY = 'insufficientFunds' export const GAS_LIMIT_TOO_LOW_ERROR_KEY = 'gasLimitTooLow' export const TRANSACTION_ERROR_KEY = 'transactionError' +export const TRANSACTION_NO_CONTRACT_ERROR_KEY = 'transactionErrorNoContract' diff --git a/ui/app/helpers/transactions.util.js b/ui/app/helpers/transactions.util.js index f7d249e63..0eb7972d6 100644 --- a/ui/app/helpers/transactions.util.js +++ b/ui/app/helpers/transactions.util.js @@ -114,7 +114,7 @@ export function getLatestSubmittedTxWithNonce (transactions = [], nonce = '0x0') export async function isSmartContractAddress (address) { const code = await global.eth.getCode(address) - return code && code !== '0x' + return code && code !== '0x' && code !== '0x0'; // Infura will return '0x', and Ganache will return '0x0' } export function sumHexes (...args) { From 4cc0b1ef01573e1541d18bdcd89650e1db32ae9a Mon Sep 17 00:00:00 2001 From: Howard Braham Date: Fri, 28 Sep 2018 11:01:34 -0700 Subject: [PATCH 2/7] ganache-core merged my PR, so I changed some comments to clarify that ganache-core v2.2.1 and below will return the non-standard '0x0' --- app/scripts/controllers/transactions/tx-gas-utils.js | 11 ++++++----- old-ui/app/components/pending-tx.js | 2 +- ui/app/components/send/send.utils.js | 2 +- ui/app/helpers/transactions.util.js | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/scripts/controllers/transactions/tx-gas-utils.js b/app/scripts/controllers/transactions/tx-gas-utils.js index 5ec728085..ac57dfe1d 100644 --- a/app/scripts/controllers/transactions/tx-gas-utils.js +++ b/app/scripts/controllers/transactions/tx-gas-utils.js @@ -63,14 +63,15 @@ class TxGasUtil { const hasRecipient = Boolean(recipient) if (hasRecipient) { - let code = await this.query.getCode(recipient) + const code = await this.query.getCode(recipient) // If there's data in the params, but there's no code, it's not a valid contract - // For no code, Infura will return '0x', and Ganache will return '0x0' + // For no code, Infura will return '0x', and ganache-core v2.2.1 will return '0x0' if (txParams.data && (!code || code === '0x' || code === '0x0')) { - throw {errorKey: TRANSACTION_NO_CONTRACT_ERROR_KEY} - } - else if (!code) { + const err = new Error() + err.errorKey = TRANSACTION_NO_CONTRACT_ERROR_KEY + throw err + } else if (!code) { txParams.gas = SIMPLE_GAS_COST // For a standard ETH send, gas is 21k max txMeta.simpleSend = true // Prevents buffer addition return SIMPLE_GAS_COST diff --git a/old-ui/app/components/pending-tx.js b/old-ui/app/components/pending-tx.js index 7d8c94699..d8d2334a4 100644 --- a/old-ui/app/components/pending-tx.js +++ b/old-ui/app/components/pending-tx.js @@ -489,7 +489,7 @@ PendingTx.prototype.verifyGasParams = function () { } PendingTx.prototype._notZeroOrEmptyString = function (obj) { - return obj !== '' && obj !== '0x0' && obj !== '0x' // The '0x' case might not ever happen, but it seems safest to protect against it + return obj !== '' && obj !== '0x0' && obj !== '0x' // '0x' means null } PendingTx.prototype.bnMultiplyByFraction = function (targetBN, numerator, denominator) { diff --git a/ui/app/components/send/send.utils.js b/ui/app/components/send/send.utils.js index 05ba6b88f..af7b3823f 100644 --- a/ui/app/components/send/send.utils.js +++ b/ui/app/components/send/send.utils.js @@ -215,7 +215,7 @@ async function estimateGas ({ // if recipient has no code, gas is 21k max: if (!selectedToken && !data) { const code = Boolean(to) && await global.eth.getCode(to) - if (!code || code === '0x' || code === '0x0') { // Infura will return '0x', and Ganache will return '0x0' + if (!code || code === '0x' || code === '0x0') { // Infura will return '0x', and ganache-core v2.2.1 will return '0x0' return SIMPLE_GAS_COST } } else if (selectedToken && !to) { diff --git a/ui/app/helpers/transactions.util.js b/ui/app/helpers/transactions.util.js index 0eb7972d6..b2c617384 100644 --- a/ui/app/helpers/transactions.util.js +++ b/ui/app/helpers/transactions.util.js @@ -114,7 +114,7 @@ export function getLatestSubmittedTxWithNonce (transactions = [], nonce = '0x0') export async function isSmartContractAddress (address) { const code = await global.eth.getCode(address) - return code && code !== '0x' && code !== '0x0'; // Infura will return '0x', and Ganache will return '0x0' + return code && code !== '0x' && code !== '0x0' // Infura will return '0x', and ganache-core v2.2.1 will return '0x0' } export function sumHexes (...args) { From 600f755dbf8d4cfdc152e3d521b537ee9a046a35 Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 9 Oct 2018 23:17:05 -0400 Subject: [PATCH 3/7] tx-gas-utils - improve format + comments --- .../controllers/transactions/tx-gas-utils.js | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/app/scripts/controllers/transactions/tx-gas-utils.js b/app/scripts/controllers/transactions/tx-gas-utils.js index ac57dfe1d..436900715 100644 --- a/app/scripts/controllers/transactions/tx-gas-utils.js +++ b/app/scripts/controllers/transactions/tx-gas-utils.js @@ -62,28 +62,34 @@ class TxGasUtil { const recipient = txParams.to const hasRecipient = Boolean(recipient) + // see if we can set the gas based on the recipient if (hasRecipient) { const code = await this.query.getCode(recipient) - - // If there's data in the params, but there's no code, it's not a valid contract - // For no code, Infura will return '0x', and ganache-core v2.2.1 will return '0x0' - if (txParams.data && (!code || code === '0x' || code === '0x0')) { - const err = new Error() - err.errorKey = TRANSACTION_NO_CONTRACT_ERROR_KEY - throw err - } else if (!code) { - txParams.gas = SIMPLE_GAS_COST // For a standard ETH send, gas is 21k max - txMeta.simpleSend = true // Prevents buffer addition + // For an address with no code, geth will return '0x', and ganache-core v2.2.1 will return '0x0' + const codeIsEmpty = !code || code === '0x' || code === '0x0' + + if (codeIsEmpty) { + // if there's data in the params, but there's no contract code, it's not a valid transaction + if (txParams.data) { + const err = new Error() + err.errorKey = TRANSACTION_NO_CONTRACT_ERROR_KEY + throw err + } + + // This is a standard ether simple send, gas requirement is exactly 21k + txParams.gas = SIMPLE_GAS_COST + // prevents buffer addition + txMeta.simpleSend = true return SIMPLE_GAS_COST } } - // if not, fall back to block gasLimit + // fallback to block gasLimit const blockGasLimitBN = hexToBn(blockGasLimitHex) const saferGasLimitBN = BnMultiplyByFraction(blockGasLimitBN, 19, 20) txParams.gas = bnToHex(saferGasLimitBN) - // run tx + // estimate tx gas requirements return await this.query.estimateGas(txParams) } From fda101912bbb99f7f1adbac9856d34105390c408 Mon Sep 17 00:00:00 2001 From: kumavis Date: Sun, 21 Oct 2018 00:52:41 -0400 Subject: [PATCH 4/7] ui - use variable to clarify result of emptiness check --- old-ui/app/components/pending-tx.js | 6 ++++-- ui/app/components/send/send.utils.js | 4 +++- ui/app/helpers/transactions.util.js | 4 +++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/old-ui/app/components/pending-tx.js b/old-ui/app/components/pending-tx.js index d8d2334a4..b02476f46 100644 --- a/old-ui/app/components/pending-tx.js +++ b/old-ui/app/components/pending-tx.js @@ -488,8 +488,10 @@ PendingTx.prototype.verifyGasParams = function () { ) } -PendingTx.prototype._notZeroOrEmptyString = function (obj) { - return obj !== '' && obj !== '0x0' && obj !== '0x' // '0x' means null +PendingTx.prototype._notZeroOrEmptyString = function (value) { + // Geth will return '0x', and ganache-core v2.2.1 will return '0x0' + const valueIsEmpty = !value || value === '0x' || value === '0x0' + return !valueIsEmpty } PendingTx.prototype.bnMultiplyByFraction = function (targetBN, numerator, denominator) { diff --git a/ui/app/components/send/send.utils.js b/ui/app/components/send/send.utils.js index af7b3823f..eb1667c63 100644 --- a/ui/app/components/send/send.utils.js +++ b/ui/app/components/send/send.utils.js @@ -215,7 +215,9 @@ async function estimateGas ({ // if recipient has no code, gas is 21k max: if (!selectedToken && !data) { const code = Boolean(to) && await global.eth.getCode(to) - if (!code || code === '0x' || code === '0x0') { // Infura will return '0x', and ganache-core v2.2.1 will return '0x0' + // Geth will return '0x', and ganache-core v2.2.1 will return '0x0' + const codeIsEmpty = !code || code === '0x' || code === '0x0' + if (codeIsEmpty) { return SIMPLE_GAS_COST } } else if (selectedToken && !to) { diff --git a/ui/app/helpers/transactions.util.js b/ui/app/helpers/transactions.util.js index 9be77e14f..cfe2c4229 100644 --- a/ui/app/helpers/transactions.util.js +++ b/ui/app/helpers/transactions.util.js @@ -114,7 +114,9 @@ export function getLatestSubmittedTxWithNonce (transactions = [], nonce = '0x0') export async function isSmartContractAddress (address) { const code = await global.eth.getCode(address) - return code && code !== '0x' && code !== '0x0' // Infura will return '0x', and ganache-core v2.2.1 will return '0x0' + // Geth will return '0x', and ganache-core v2.2.1 will return '0x0' + const codeIsEmpty = !code || code === '0x' || code === '0x0' + return !codeIsEmpty } export function sumHexes (...args) { From 17a856cfd35a0e419524cf00e852be6c797b1e0b Mon Sep 17 00:00:00 2001 From: kumavis Date: Sun, 21 Oct 2018 00:57:45 -0400 Subject: [PATCH 5/7] send tx - validate - simplify error message for attempting to call function on non-contract address --- app/_locales/en/messages.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index 4cdf6b8dc..e2fb3fc13 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -1176,7 +1176,7 @@ "message": "Transaction Error. Exception thrown in contract code." }, "transactionErrorNoContract": { - "message": "Trying to call a contract function at an address that is not a contract address." + "message": "Trying to call a function on a non-contract address." }, "transactionMemo": { "message": "Transaction memo (optional)" From 31e5cad1e34c1b07079c430bb1903f7914021111 Mon Sep 17 00:00:00 2001 From: kumavis Date: Sun, 21 Oct 2018 01:01:21 -0400 Subject: [PATCH 6/7] tx-gas-util - set error message when invalidating tx based on tx data but no contract code --- app/scripts/controllers/transactions/tx-gas-utils.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/scripts/controllers/transactions/tx-gas-utils.js b/app/scripts/controllers/transactions/tx-gas-utils.js index 436900715..def67c2c3 100644 --- a/app/scripts/controllers/transactions/tx-gas-utils.js +++ b/app/scripts/controllers/transactions/tx-gas-utils.js @@ -62,20 +62,21 @@ class TxGasUtil { const recipient = txParams.to const hasRecipient = Boolean(recipient) - // see if we can set the gas based on the recipient + // see if we can set the gas based on the recipient if (hasRecipient) { const code = await this.query.getCode(recipient) // For an address with no code, geth will return '0x', and ganache-core v2.2.1 will return '0x0' const codeIsEmpty = !code || code === '0x' || code === '0x0' - + if (codeIsEmpty) { // if there's data in the params, but there's no contract code, it's not a valid transaction if (txParams.data) { - const err = new Error() + const err = new Error('TxGasUtil - Trying to call a function on a non-contract address') + // set error key so ui can display localized error message err.errorKey = TRANSACTION_NO_CONTRACT_ERROR_KEY throw err } - + // This is a standard ether simple send, gas requirement is exactly 21k txParams.gas = SIMPLE_GAS_COST // prevents buffer addition From 9b501b7c42ebebb61ac3130d1e84d36efcac9b7e Mon Sep 17 00:00:00 2001 From: kumavis Date: Thu, 25 Oct 2018 22:24:08 -0400 Subject: [PATCH 7/7] old-ui - pending tx - allow undefined values for gas + gasPrice --- old-ui/app/components/pending-tx.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/old-ui/app/components/pending-tx.js b/old-ui/app/components/pending-tx.js index b02476f46..35e81210e 100644 --- a/old-ui/app/components/pending-tx.js +++ b/old-ui/app/components/pending-tx.js @@ -1,4 +1,5 @@ const Component = require('react').Component +const connect = require('react-redux').connect const h = require('react-hyperscript') const inherits = require('util').inherits const actions = require('../../../ui/app/actions') @@ -19,7 +20,9 @@ const BNInput = require('./bn-as-decimal-input') const MIN_GAS_PRICE_BN = new BN('0') const MIN_GAS_LIMIT_BN = new BN('21000') -module.exports = PendingTx +module.exports = connect()(PendingTx) + + inherits(PendingTx, Component) function PendingTx () { Component.call(this) @@ -445,7 +448,8 @@ PendingTx.prototype.onSubmit = function (event) { const txMeta = this.gatherTxMeta() const valid = this.checkValidity() this.setState({ valid, submitting: true }) - if (valid && this.verifyGasParams()) { + const validGasParams = this.verifyGasParams() + if (valid && validGasParams) { this.props.sendTransaction(txMeta, event) } else { this.props.dispatch(actions.displayWarning('Invalid Gas Parameters')) @@ -489,6 +493,8 @@ PendingTx.prototype.verifyGasParams = function () { } PendingTx.prototype._notZeroOrEmptyString = function (value) { + // allow undefined values + if (value === undefined) return true // Geth will return '0x', and ganache-core v2.2.1 will return '0x0' const valueIsEmpty = !value || value === '0x' || value === '0x0' return !valueIsEmpty