Fix default gas race condition (#8490)

A race condition exists where after adding an unapproved transaction,
it could be mutated and then replaced when the default gas parameters
are set. This happens because the transaction is added to state and
broadcast before the default gas parameters are set, because
calculating the default gas parameters to use takes some time.
Once they've been calculated, the false assumption was made that the
transaction hadn't changed.

The method responsible for setting the default gas now retrieves an
up-to-date copy of `txMeta`, and conditionally sets the defaults only
if they haven't yet been set.

This race condition was introduced in #2962, though that PR also added
a loading screen that avoided this issue by preventing the user from
interacting with the transaction until after the gas had been
estimated. Unfortunately this loading screen was not carried forward to
the new UI.

app/scripts/controllers/transactions/index.js

@@ -247,13 +247,13 @@ export default class TransactionController extends EventEmitter {
       txMeta = await this.addTxGasDefaults(txMeta, getCodeResponse)
     } catch (error) {
       log.warn(error)
+      txMeta = this.txStateManager.getTx(txMeta.id)
       txMeta.loadingDefaults = false
       this.txStateManager.updateTx(txMeta, 'Failed to calculate gas defaults.')
       throw error
     }
 
     txMeta.loadingDefaults = false
-
     // save txMeta
     this.txStateManager.updateTx(txMeta, 'Added new unapproved transaction.')
 
@@ -266,24 +266,53 @@ export default class TransactionController extends EventEmitter {
    * @returns {Promise<object>} - resolves with txMeta
    */
   async addTxGasDefaults (txMeta, getCodeResponse) {
-    const txParams = txMeta.txParams
+    const defaultGasPrice = await this._getDefaultGasPrice(txMeta)
+    const { gasLimit: defaultGasLimit, simulationFails } = await this._getDefaultGasLimit(txMeta, getCodeResponse)
 
-    let gasPrice = txParams.gasPrice
-    if (!gasPrice) {
-      gasPrice = this.getGasPrice ? this.getGasPrice() : await this.query.gasPrice()
+    txMeta = this.txStateManager.getTx(txMeta.id)
+    if (simulationFails) {
+      txMeta.simulationFails = simulationFails
     }
-    txParams.gasPrice = ethUtil.addHexPrefix(gasPrice.toString(16))
+    if (defaultGasPrice && !txMeta.txParams.gasPrice) {
+      txMeta.txParams.gasPrice = defaultGasPrice
+    }
+    if (defaultGasLimit && !txMeta.txParams.gas) {
+      txMeta.txParams.gas = defaultGasLimit
+    }
+    return txMeta
+  }
 
-    // set gasLimit
+  /**
+   * Gets default gas price, or returns `undefined` if gas price is already set
+   * @param {Object} txMeta - The txMeta object
+   * @returns {Promise<string>} The default gas price
+   */
+  async _getDefaultGasPrice (txMeta) {
+    if (txMeta.txParams.gasPrice) {
+      return
+    }
+    const gasPrice = this.getGasPrice
+      ? this.getGasPrice()
+      : await this.query.gasPrice()
 
-    if (txParams.gas) {
-      return txMeta
+    return ethUtil.addHexPrefix(gasPrice.toString(16))
+  }
+
+  /**
+   * Gets default gas limit, or debug information about why gas estimate failed.
+   * @param {Object} txMeta - The txMeta object
+   * @param {string} getCodeResponse - The transaction category code response, used for debugging purposes
+   * @returns {Promise<Object>} Object containing the default gas limit, or the simulation failure object
+   */
+  async _getDefaultGasLimit (txMeta, getCodeResponse) {
+    if (txMeta.txParams.gas) {
+      return {}
     } else if (
-      txParams.to &&
+      txMeta.txParams.to &&
       txMeta.transactionCategory === SEND_ETHER_ACTION_KEY
     ) {
       // if there's data in the params, but there's no contract code, it's not a valid transaction
-      if (txParams.data) {
+      if (txMeta.txParams.data) {
         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
@@ -294,17 +323,14 @@ export default class TransactionController extends EventEmitter {
       }
 
       // This is a standard ether simple send, gas requirement is exactly 21k
-      txParams.gas = SIMPLE_GAS_COST
-      return txMeta
+      return { gasLimit: SIMPLE_GAS_COST }
     }
 
     const { blockGasLimit, estimatedGasHex, simulationFails } = await this.txGasUtil.analyzeGasUsage(txMeta)
-    if (simulationFails) {
-      txMeta.simulationFails = simulationFails
-    } else {
-      this.txGasUtil.setTxGas(txMeta, blockGasLimit, estimatedGasHex)
-    }
-    return txMeta
+
+    // add additional gas buffer to our estimation for safety
+    const gasLimit = this.txGasUtil.addGasBuffer(ethUtil.addHexPrefix(estimatedGasHex), blockGasLimit)
+    return { gasLimit, simulationFails }
   }
 
   /**
@@ -647,14 +673,16 @@ export default class TransactionController extends EventEmitter {
       status: 'unapproved',
       loadingDefaults: true,
     }).forEach((tx) => {
+
       this.addTxGasDefaults(tx)
         .then((txMeta) => {
           txMeta.loadingDefaults = false
           this.txStateManager.updateTx(txMeta, 'transactions: gas estimation for tx on boot')
         }).catch((error) => {
-          tx.loadingDefaults = false
-          this.txStateManager.updateTx(tx, 'failed to estimate gas during boot cleanup.')
-          this.txStateManager.setTxStatusFailed(tx.id, error)
+          const txMeta = this.txStateManager.getTx(tx.id)
+          txMeta.loadingDefaults = false
+          this.txStateManager.updateTx(txMeta, 'failed to estimate gas during boot cleanup.')
+          this.txStateManager.setTxStatusFailed(txMeta.id, error)
         })
     })
 

app/scripts/controllers/transactions/tx-gas-utils.js

@@ -1,7 +1,6 @@
 import EthQuery from 'ethjs-query'
 import { hexToBn, BnMultiplyByFraction, bnToHex } from '../../lib/util'
 import log from 'loglevel'
-import { addHexPrefix } from 'ethereumjs-util'
 
 /**
  * Result of gas analysis, including either a gas estimate for a successful analysis, or
@@ -31,15 +30,19 @@ export default class TxGasUtil {
   */
   async analyzeGasUsage (txMeta) {
     const block = await this.query.getBlockByNumber('latest', false)
-    let estimatedGasHex
+
+    // fallback to block gasLimit
+    const blockGasLimitBN = hexToBn(block.gasLimit)
+    const saferGasLimitBN = BnMultiplyByFraction(blockGasLimitBN, 19, 20)
+    let estimatedGasHex = bnToHex(saferGasLimitBN)
     let simulationFails
     try {
       estimatedGasHex = await this.estimateTxGas(txMeta, block.gasLimit)
-    } catch (err) {
-      log.warn(err)
+    } catch (error) {
+      log.warn(error)
       simulationFails = {
-        reason: err.message,
-        errorKey: err.errorKey,
+        reason: error.message,
+        errorKey: error.errorKey,
         debug: { blockNumber: block.number, blockGasLimit: block.gasLimit },
       }
     }
@@ -50,37 +53,15 @@ export default class TxGasUtil {
   /**
     Estimates the tx's gas usage
     @param {Object} txMeta - the txMeta object
-    @param {string} blockGasLimitHex - hex string of the block's gas limit
     @returns {string} - the estimated gas limit as a hex string
   */
-  async estimateTxGas (txMeta, blockGasLimitHex) {
+  async estimateTxGas (txMeta) {
     const txParams = txMeta.txParams
 
-    // fallback to block gasLimit
-    const blockGasLimitBN = hexToBn(blockGasLimitHex)
-    const saferGasLimitBN = BnMultiplyByFraction(blockGasLimitBN, 19, 20)
-    txParams.gas = bnToHex(saferGasLimitBN)
-
     // estimate tx gas requirements
     return await this.query.estimateGas(txParams)
   }
 
-  /**
-    Writes the gas on the txParams in the txMeta
-    @param {Object} txMeta - the txMeta object to write to
-    @param {string} blockGasLimitHex - the block gas limit hex
-    @param {string} estimatedGasHex - the estimated gas hex
-  */
-  setTxGas (txMeta, blockGasLimitHex, estimatedGasHex) {
-    const txParams = txMeta.txParams
-
-    // if gasLimit not originally specified,
-    // try adding an additional gas buffer to our estimation for safety
-    const recommendedGasHex = this.addGasBuffer(addHexPrefix(estimatedGasHex), blockGasLimitHex)
-    txParams.gas = recommendedGasHex
-    return
-  }
-
   /**
     Adds a gas buffer with out exceeding the block gas limit
 

test/unit/app/controllers/transactions/tx-controller-test.js

@@ -230,7 +230,11 @@ describe('Transaction Controller', function () {
 
   describe('#addTxGasDefaults', function () {
     it('should add the tx defaults if their are none', async function () {
+      txController.txStateManager._saveTxList([
+        { id: 1, status: 'unapproved', metamaskNetworkId: currentNetworkId, txParams: {}, history: [{}] },
+      ])
       const txMeta = {
+        id: 1,
         txParams: {
           from: '0xc684832530fcbddae4b4230a47e991ddcec2831d',
           to: '0xc684832530fcbddae4b4230a47e991ddcec2831d',