Fix signing method bugs (#8833)

* update signTypedData validation

* update tests for new eth-json-rpc-middleware

* remove lowercasing of tx 'from' addresses

app/scripts/controllers/permissions/enums.js

@@ -75,6 +75,7 @@ export const SAFE_METHODS = [
   'eth_sendTransaction',
   'eth_sign',
   'personal_sign',
+  'personal_ecRecover',
   'eth_signTypedData',
   'eth_signTypedData_v1',
   'eth_signTypedData_v3',

app/scripts/controllers/transactions/lib/util.js

@@ -1,8 +1,8 @@
 import { addHexPrefix, isValidAddress } from 'ethereumjs-util'
 
 const normalizers = {
-  from: (from, lowerCase = true) => (lowerCase ? addHexPrefix(from).toLowerCase() : addHexPrefix(from)),
+  from: (from) => addHexPrefix(from),
-  to: (to, lowerCase = true) => (lowerCase ? addHexPrefix(to).toLowerCase() : addHexPrefix(to)),
+  to: (to, lowerCase) => (lowerCase ? addHexPrefix(to).toLowerCase() : addHexPrefix(to)),
   nonce: (nonce) => addHexPrefix(nonce),
   value: (value) => addHexPrefix(value),
   data: (data) => addHexPrefix(data),
@@ -12,11 +12,12 @@ const normalizers = {
 
 /**
  * Normalizes the given txParams
- * @param {Object} txParams - the tx params
+ * @param {Object} txParams - The transaction params
- * @param {boolean} lowerCase - whether to return the addresses lower cased
+ * @param {boolean} [lowerCase] - Whether to lowercase the 'to' address.
+ * Default: true
  * @returns {Object} the normalized tx params
  */
-export function normalizeTxParams (txParams, lowerCase) {
+export function normalizeTxParams (txParams, lowerCase = true) {
   // apply only keys in the normalizers
   const normalizedTxParams = {}
   for (const key in normalizers) {

app/scripts/lib/encryption-public-key-manager.js

@@ -283,5 +283,4 @@ export default class EncryptionPublicKeyManager extends EventEmitter {
     this.memStore.updateState({ unapprovedEncryptionPublicKeyMsgs, unapprovedEncryptionPublicKeyMsgCount })
     this.emit('updateBadge')
   }
-
 }

app/scripts/lib/typed-message-manager.js

@@ -4,9 +4,11 @@ import createId from './random-id'
 import assert from 'assert'
 import { ethErrors } from 'eth-json-rpc-errors'
 import sigUtil from 'eth-sig-util'
+import { isValidAddress } from 'ethereumjs-util'
 import log from 'loglevel'
 import jsonschema from 'jsonschema'
 import { MESSAGE_TYPE } from './enums'
+
 /**
  * Represents, and contains data about, an 'eth_signTypedData' type signature request. These are created when a
  * signature for an eth_signTypedData call is requested.
@@ -102,14 +104,15 @@ export default class TypedMessageManager extends EventEmitter {
    *
    */
   addUnapprovedMessage (msgParams, req, version) {
+
     msgParams.version = version
-    this.validateParams(msgParams)
-    // add origin from request
     if (req) {
       msgParams.origin = req.origin
     }
+    this.validateParams(msgParams)
 
     log.debug(`TypedMessageManager addUnapprovedMessage: ${JSON.stringify(msgParams)}`)
+
     // create txData obj with parameters and meta data
     const time = (new Date()).getTime()
     const msgId = createId()
@@ -134,37 +137,38 @@ export default class TypedMessageManager extends EventEmitter {
    *
    */
   validateParams (params) {
+
+    assert.ok(params && typeof params === 'object', 'Params must be an object.')
+    assert.ok('data' in params, 'Params must include a "data" field.')
+    assert.ok('from' in params, 'Params must include a "from" field.')
+    assert.ok(
+      typeof params.from === 'string' && isValidAddress(params.from),
+      '"from" field must be a valid, lowercase, hexadecimal Ethereum address string.'
+    )
+
     switch (params.version) {
       case 'V1':
-        assert.equal(typeof params, 'object', 'Params should ben an object.')
+        assert.ok(Array.isArray(params.data), '"params.data" must be an array.')
-        assert.ok('data' in params, 'Params must include a data field.')
-        assert.ok('from' in params, 'Params must include a from field.')
-        assert.ok(Array.isArray(params.data), 'Data should be an array.')
-        assert.equal(typeof params.from, 'string', 'From field must be a string.')
         assert.doesNotThrow(() => {
           sigUtil.typedSignatureHash(params.data)
-        }, 'Expected EIP712 typed data')
+        }, 'Signing data must be valid EIP-712 typed data.')
         break
       case 'V3':
       case 'V4':
+        assert.equal(typeof params.data, 'string', '"params.data" must be a string.')
         let data
-        assert.equal(typeof params, 'object', 'Params should be an object.')
-        assert.ok('data' in params, 'Params must include a data field.')
-        assert.ok('from' in params, 'Params must include a from field.')
-        assert.equal(typeof params.from, 'string', 'From field must be a string.')
-        assert.equal(typeof params.data, 'string', 'Data must be passed as a valid JSON string.')
         assert.doesNotThrow(() => {
           data = JSON.parse(params.data)
-        }, 'Data must be passed as a valid JSON string.')
+        }, '"data" must be a valid JSON string.')
         const validation = jsonschema.validate(data, sigUtil.TYPED_MESSAGE_SCHEMA)
         assert.ok(data.primaryType in data.types, `Primary type of "${data.primaryType}" has no type definition.`)
-        assert.equal(validation.errors.length, 0, 'Data must conform to EIP-712 schema. See https://git.io/fNtcx.')
+        assert.equal(validation.errors.length, 0, 'Signing data must conform to EIP-712 schema. See https://git.io/fNtcx.')
         const chainId = data.domain.chainId
         const activeChainId = parseInt(this.networkController.getNetworkState())
-        chainId && assert.equal(chainId, activeChainId, `Provided chainId (${chainId}) must match the active chainId (${activeChainId})`)
+        chainId && assert.equal(chainId, activeChainId, `Provided chainId "${chainId}" must match the active chainId "${activeChainId}"`)
         break
       default:
-        assert.fail(`Unknown params.version ${params.version}`)
+        assert.fail(`Unknown typed data version "${params.version}"`)
     }
   }
 

package.json

@@ -106,7 +106,7 @@
     "eth-json-rpc-errors": "^2.0.2",
     "eth-json-rpc-filters": "^4.1.1",
     "eth-json-rpc-infura": "^4.0.2",
-    "eth-json-rpc-middleware": "^4.4.1",
+    "eth-json-rpc-middleware": "^5.0.0",
     "eth-keyring-controller": "^6.0.0",
     "eth-method-registry": "^1.2.0",
     "eth-phishing-detect": "^1.1.4",

test/unit/app/controllers/metamask-controller-test.js

@@ -35,9 +35,33 @@ const ExtensionizerMock = {
   },
 }
 
+let loggerMiddlewareMock
+const initializeMockMiddlewareLog = () => {
+  loggerMiddlewareMock = {
+    requests: [],
+    responses: [],
+  }
+}
+const tearDownMockMiddlewareLog = () => {
+  loggerMiddlewareMock = undefined
+}
+
+const createLoggerMiddlewareMock = () => (req, res, next) => {
+  if (loggerMiddlewareMock) {
+    loggerMiddlewareMock.requests.push(req)
+    next((cb) => {
+      loggerMiddlewareMock.responses.push(res)
+      cb()
+    })
+  } else {
+    next()
+  }
+}
+
 const MetaMaskController = proxyquire('../../../../app/scripts/metamask-controller', {
   './controllers/threebox': { default: ThreeBoxControllerMock },
   'extensionizer': ExtensionizerMock,
+  './lib/createLoggerMiddleware': { default: createLoggerMiddlewareMock },
 }).default
 
 const currentNetworkId = 42
@@ -96,7 +120,6 @@ describe('MetaMaskController', function () {
     // add sinon method spies
     sandbox.spy(metamaskController.keyringController, 'createNewVaultAndKeychain')
     sandbox.spy(metamaskController.keyringController, 'createNewVaultAndRestore')
-    sandbox.spy(metamaskController.txController, 'newUnapprovedTransaction')
   })
 
   afterEach(function () {
@@ -776,6 +799,17 @@ describe('MetaMaskController', function () {
   })
 
   describe('#setupUntrustedCommunication', function () {
+
+    const mockTxParams = { from: TEST_ADDRESS }
+
+    beforeEach(function () {
+      initializeMockMiddlewareLog()
+    })
+
+    after(function () {
+      tearDownMockMiddlewareLog()
+    })
+
     it('sets up phishing stream for untrusted communication', async function () {
       const phishingMessageSender = {
         url: 'http://myethereumwalletntw.com',
@@ -815,7 +849,7 @@ describe('MetaMaskController', function () {
       const message = {
         id: 1999133338649204,
         jsonrpc: '2.0',
-        params: ['mock tx params'],
+        params: [{ ...mockTxParams }],
         method: 'eth_sendTransaction',
       }
       streamTest.write({
@@ -824,15 +858,12 @@ describe('MetaMaskController', function () {
       }, null, () => {
         setTimeout(() => {
           assert.deepStrictEqual(
-            metamaskController.txController.newUnapprovedTransaction.getCall(0).args,
+            loggerMiddlewareMock.requests[0],
-            [
-              'mock tx params',
             {
               ...message,
               origin: 'http://mycrypto.com',
               tabId: 456,
             },
-            ]
           )
           done()
         })
@@ -856,7 +887,7 @@ describe('MetaMaskController', function () {
       const message = {
         id: 1999133338649204,
         jsonrpc: '2.0',
-        params: ['mock tx params'],
+        params: [{ ...mockTxParams }],
         method: 'eth_sendTransaction',
       }
       streamTest.write({
@@ -865,14 +896,11 @@ describe('MetaMaskController', function () {
       }, null, () => {
         setTimeout(() => {
           assert.deepStrictEqual(
-            metamaskController.txController.newUnapprovedTransaction.getCall(0).args,
+            loggerMiddlewareMock.requests[0],
-            [
-              'mock tx params',
             {
               ...message,
               origin: 'http://mycrypto.com',
             },
-            ]
           )
           done()
         })

yarn.lock

@@ -10174,7 +10174,7 @@ eth-json-rpc-middleware@^1.5.0:
     promise-to-callback "^1.0.0"
     tape "^4.6.3"
 
-eth-json-rpc-middleware@^4.1.4, eth-json-rpc-middleware@^4.1.5, eth-json-rpc-middleware@^4.4.1:
+eth-json-rpc-middleware@^4.1.4, eth-json-rpc-middleware@^4.1.5:
   version "4.4.1"
   resolved "https://registry.yarnpkg.com/eth-json-rpc-middleware/-/eth-json-rpc-middleware-4.4.1.tgz#07d3dd0724c24a8d31e4a172ee96271da71b4228"
   integrity sha512-yoSuRgEYYGFdVeZg3poWOwAlRI+MoBIltmOB86MtpoZjvLbou9EB/qWMOWSmH2ryCWLW97VYY6NWsmWm3OAA7A==
@@ -10194,6 +10194,26 @@ eth-json-rpc-middleware@^4.1.4, eth-json-rpc-middleware@^4.1.5, eth-json-rpc-mid
     pify "^3.0.0"
     safe-event-emitter "^1.0.1"
 
+eth-json-rpc-middleware@^5.0.0:
+  version "5.0.0"
+  resolved "https://registry.yarnpkg.com/eth-json-rpc-middleware/-/eth-json-rpc-middleware-5.0.0.tgz#9a202a41a2b1678c094cbffd0db0da619ec86f7b"
+  integrity sha512-31zLHvElrQV7opFZs47UqjMGFM4QCgMms7HJSQAZ3wjPuE3rFLxFUTeGHXwM3fAp9zh7n0M+B2gK7Ds4OUKfog==
+  dependencies:
+    btoa "^1.2.1"
+    clone "^2.1.1"
+    eth-query "^2.1.2"
+    eth-rpc-errors "^2.1.1"
+    eth-sig-util "^1.4.2"
+    ethereumjs-block "^1.6.0"
+    ethereumjs-tx "^1.3.7"
+    ethereumjs-util "^5.1.2"
+    ethereumjs-vm "^2.6.0"
+    fetch-ponyfill "^4.0.0"
+    json-rpc-engine "^5.1.3"
+    json-stable-stringify "^1.0.1"
+    pify "^3.0.0"
+    safe-event-emitter "^1.0.1"
+
 eth-keyring-controller@^5.3.0, eth-keyring-controller@^5.6.1:
   version "5.6.1"
   resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-5.6.1.tgz#7b7268400704c8f5ce98a055910341177dd207ca"
@@ -10275,6 +10295,13 @@ eth-query@^2.0.2, eth-query@^2.1.0, eth-query@^2.1.2:
     json-rpc-random-id "^1.0.0"
     xtend "^4.0.1"
 
+eth-rpc-errors@^2.1.1:
+  version "2.1.1"
+  resolved "https://registry.yarnpkg.com/eth-rpc-errors/-/eth-rpc-errors-2.1.1.tgz#00a7d6c8a9c864a8ab7d0356be20964e5bee4b13"
+  integrity sha512-MY3zAa5ZF8hvgQu1HOF9agaK5GgigBRGpTJ8H0oVlE0NqMu13CW6syyjLXdeIDCGQTbUeHliU1z9dVmvMKx1Tg==
+  dependencies:
+    fast-safe-stringify "^2.0.6"
+
 eth-sig-util@2.3.0, eth-sig-util@^2.3.0:
   version "2.3.0"
   resolved "https://registry.yarnpkg.com/eth-sig-util/-/eth-sig-util-2.3.0.tgz#c54a6ac8e8796f7e25f59cf436982a930e645231"