Merge pull request #3831 from MetaMask/i#3770

some more transaction validation bug fixes

app/scripts/controllers/transactions.js

@@ -187,12 +187,12 @@ module.exports = class TransactionController extends EventEmitter {
     // validate
     await this.txGasUtil.validateTxParams(txParams)
     // construct txMeta
-    const txMeta = this.txStateManager.generateTxMeta({txParams})
+    let txMeta = this.txStateManager.generateTxMeta({txParams})
     this.addTx(txMeta)
     this.emit('newUnapprovedTx', txMeta)
     // add default tx params
     try {
-      await this.addTxDefaults(txMeta)
+      txMeta = await this.addTxDefaults(txMeta)
     } catch (error) {
       console.log(error)
       this.txStateManager.setTxStatusFailed(txMeta.id, error)
@@ -215,6 +215,7 @@ module.exports = class TransactionController extends EventEmitter {
     }
     txParams.gasPrice = ethUtil.addHexPrefix(gasPrice.toString(16))
     txParams.value = txParams.value || '0x0'
+    if (txParams.to === null) delete txParams.to
     // set gasLimit
     return await this.txGasUtil.analyzeGasUsage(txMeta)
   }

app/scripts/lib/tx-gas-utils.js

@@ -52,7 +52,9 @@ module.exports = class TxGasUtil {
     // if recipient has no code, gas is 21k max:
     const recipient = txParams.to
     const hasRecipient = Boolean(recipient)
-    const code = await this.query.getCode(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
@@ -100,6 +102,7 @@ module.exports = class TxGasUtil {
   }
 
   async validateTxParams (txParams) {
+    this.validateFrom(txParams)
     this.validateRecipient(txParams)
     if ('value' in txParams) {
       const value = txParams.value.toString()
@@ -112,6 +115,12 @@ module.exports = class TxGasUtil {
       }
     }
   }
+
+  validateFrom (txParams) {
+    if ( !(typeof txParams.from === 'string') ) throw new Error(`Invalid from address ${txParams.from} not a string`)
+    if (!isValidAddress(txParams.from)) throw new Error('Invalid from address')
+  }
+
   validateRecipient (txParams) {
     if (txParams.to === '0x' || txParams.to === null ) {
       if (txParams.data) {
@@ -124,4 +133,4 @@ module.exports = class TxGasUtil {
     }
     return txParams
   }
-}
+}

test/unit/tx-controller-test.js

@@ -162,7 +162,7 @@ describe('Transaction Controller', function () {
   describe('#addUnapprovedTransaction', function () {
 
     it('should add an unapproved transaction and return a valid txMeta', function (done) {
-      txController.addUnapprovedTransaction({})
+      txController.addUnapprovedTransaction({ from: '0x1678a085c290ebd122dc42cba69373b5953b831d' })
       .then((txMeta) => {
         assert(('id' in txMeta), 'should have a id')
         assert(('time' in txMeta), 'should have a time stamp')
@@ -182,7 +182,7 @@ describe('Transaction Controller', function () {
         assert(txMetaFromEmit, 'txMeta is falsey')
         done()
       })
-      txController.addUnapprovedTransaction({})
+      txController.addUnapprovedTransaction({ from: '0x1678a085c290ebd122dc42cba69373b5953b831d' })
       .catch(done)
     })
 
@@ -213,6 +213,7 @@ describe('Transaction Controller', function () {
   describe('#validateTxParams', function () {
     it('does not throw for positive values', function (done) {
       var sample = {
+        from: '0x1678a085c290ebd122dc42cba69373b5953b831d',
         value: '0x01',
       }
       txController.txGasUtil.validateTxParams(sample).then(() => {
@@ -222,6 +223,7 @@ describe('Transaction Controller', function () {
 
     it('returns error for negative values', function (done) {
       var sample = {
+        from: '0x1678a085c290ebd122dc42cba69373b5953b831d',
         value: '-0x01',
       }
       txController.txGasUtil.validateTxParams(sample)

test/unit/tx-gas-util-test.js

@@ -29,4 +29,28 @@ describe('Tx Gas Util', function () {
     }
     assert.throws(() => { txGasUtil.validateRecipient(zeroRecipientTxParams) }, Error, 'Invalid recipient address')
   })
+
+  it('should error when from is not a hex string', function () {
+
+    // where from is undefined
+    const txParams = {}
+    assert.throws(() => { txGasUtil.validateFrom(txParams) }, Error, `Invalid from address ${txParams.from} not a string`)
+
+    // where from is array
+    txParams.from = []
+    assert.throws(() => { txGasUtil.validateFrom(txParams) }, Error, `Invalid from address ${txParams.from} not a string`)
+
+    // where from is a object
+    txParams.from = {}
+    assert.throws(() => { txGasUtil.validateFrom(txParams) }, Error, `Invalid from address ${txParams.from} not a string`)
+
+    // where from is a invalid address
+    txParams.from = 'im going to fail'
+    assert.throws(() => { txGasUtil.validateFrom(txParams) }, Error, `Invalid from address`)
+
+    // should run
+    txParams.from ='0x1678a085c290ebd122dc42cba69373b5953b831d'
+    txGasUtil.validateFrom(txParams)
+    })
+
 })