Validate txParams in TransactionStateManager.addTx (#6713)

* Normalize and Validate txParams in TransactionStateManager.addTx too

* Added Tests

* Updated normalizeAndValidateParams to return the new txParams

app/scripts/controllers/transactions/tx-state-manager.js

@@ -125,6 +125,11 @@ class TransactionStateManager extends EventEmitter {
     @returns {object} the txMeta
   */
   addTx (txMeta) {
+    // normalize and validate txParams if present
+    if (txMeta.txParams) {
+      txMeta.txParams = this.normalizeAndValidateTxParams(txMeta.txParams)
+    }
+
     this.once(`${txMeta.id}:signed`, function () {
       this.removeAllListeners(`${txMeta.id}:rejected`)
     })
@@ -174,13 +179,9 @@ class TransactionStateManager extends EventEmitter {
     @param [note] {string} - a note about the update for history
   */
   updateTx (txMeta, note) {
-    // validate txParams
+    // normalize and validate txParams if present
     if (txMeta.txParams) {
-      if (typeof txMeta.txParams.data === 'undefined') {
-        delete txMeta.txParams.data
-      }
-      txMeta.txParams = normalizeTxParams(txMeta.txParams, false)
-      this.validateTxParams(txMeta.txParams)
+      txMeta.txParams = this.normalizeAndValidateTxParams(txMeta.txParams)
     }
 
     // create txMeta snapshot for history
@@ -212,6 +213,19 @@ class TransactionStateManager extends EventEmitter {
     this.updateTx(txMeta, `txStateManager#updateTxParams`)
   }
 
+  /**
+   * normalize and validate txParams members
+   * @param txParams {object} - txParams
+   */
+  normalizeAndValidateTxParams (txParams) {
+    if (typeof txParams.data === 'undefined') {
+      delete txParams.data
+    }
+    txParams = normalizeTxParams(txParams, false)
+    this.validateTxParams(txParams)
+    return txParams
+  }
+
   /**
     validates txParams members by type
     @param txParams {object} - txParams to validate

test/unit/app/controllers/transactions/tx-state-manager-test.js

@@ -93,6 +93,37 @@ describe('TransactionStateManager', function () {
       assert.equal(result[0].id, 1)
     })
 
+    it('throws error and does not add tx if txParams are invalid', function () {
+      const validTxParams = {
+        from: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc',
+        to: '0x0039f22efb07a647557c7c5d17854cfd6d489ef3',
+        nonce: '0x3',
+        gas: '0x77359400',
+        gasPrice: '0x77359400',
+        value: '0x0',
+        data: '0x0',
+      }
+      const invalidValues = [1, true, {}, Symbol('1')]
+
+      for (const key in validTxParams) {
+        for (const value of invalidValues) {
+          const tx = {
+            id: 1,
+            status: 'unapproved',
+            metamaskNetworkId: currentNetworkId,
+            txParams: {
+              ...validTxParams,
+              [key]: value,
+            },
+          }
+          assert.throws(txStateManager.addTx.bind(txStateManager, tx), 'addTx should throw error')
+          const result = txStateManager.getTxList()
+          assert.ok(Array.isArray(result), 'txList should be an array')
+          assert.equal(result.length, 0, 'txList should be empty')
+        }
+      }
+    })
+
     it('does not override txs from other networks', function () {
       const tx = { id: 1, status: 'confirmed', metamaskNetworkId: currentNetworkId, txParams: {} }
       const tx2 = { id: 2, status: 'confirmed', metamaskNetworkId: otherNetworkId, txParams: {} }
@@ -153,6 +184,37 @@ describe('TransactionStateManager', function () {
       assert.equal(result.hash, 'foo')
     })
 
+    it('throws error and does not update tx if txParams are invalid', function () {
+      const validTxParams = {
+        from: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc',
+        to: '0x0039f22efb07a647557c7c5d17854cfd6d489ef3',
+        nonce: '0x3',
+        gas: '0x77359400',
+        gasPrice: '0x77359400',
+        value: '0x0',
+        data: '0x0',
+      }
+      const invalidValues = [1, true, {}, Symbol('1')]
+
+      txStateManager.addTx({ id: 1, status: 'unapproved', metamaskNetworkId: currentNetworkId, txParams: validTxParams })
+
+      for (const key in validTxParams) {
+        for (const value of invalidValues) {
+          const originalTx = txStateManager.getTx(1)
+          const newTx = {
+            ...originalTx,
+            txParams: {
+              ...originalTx.txParams,
+              [key]: value,
+            },
+          }
+          assert.throws(txStateManager.updateTx.bind(txStateManager, newTx), 'updateTx should throw an error')
+          const result = txStateManager.getTx(1)
+          assert.deepEqual(result, originalTx, 'tx should not be updated')
+        }
+      }
+    })
+
     it('updates gas price and adds history items', function () {
       const originalGasPrice = '0x01'
       const desiredGasPrice = '0x02'