From f7639d6f1b05eefe023d46df310680b2f0393327 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Wed, 20 Mar 2019 18:16:36 -0300 Subject: [PATCH 1/4] security audit suggestion --- app/util/transactions.js | 26 +++++++++++++++---------- app/util/transactions.test.js | 36 +++++++++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/app/util/transactions.js b/app/util/transactions.js index 7a875d86708..d150f103e2c 100644 --- a/app/util/transactions.js +++ b/app/util/transactions.js @@ -140,24 +140,30 @@ export function getMethodData(data) { * Returns wether the given address is a contract * * @param {string} address - Ethereum address + * @param {string} transactionHash? - Transaction hash * @returns {boolean} - Wether the given address is a contract */ -export async function isSmartContractAddress(address) { - const cache = SmartContractAddresses.cache[address]; - if (cache) { - return Promise.resolve(cache); - } - +export async function isSmartContractAddress(address, transactionHash) { + address = toChecksumAddress(address); + // If in contract map we don't need to cache it if (contractMap[address]) { - SmartContractAddresses.cache[address] = true; return Promise.resolve(true); } - + // Look in cache memory only for specific time (transaction hash) + if (transactionHash) { + const cache = SmartContractAddresses.cache[(address, transactionHash)]; + if (cache) { + return Promise.resolve(cache); + } + } const { TransactionController } = Engine.context; const code = address ? await TransactionController.query('getCode', [address]) : undefined; // Geth will return '0x', and ganache-core v2.2.1 will return '0x0' const codeIsEmpty = !code || code === '0x' || code === '0x0'; - SmartContractAddresses.cache[address] = !codeIsEmpty; + // Save in cache for specific time (transaction hash) + if (transactionHash) { + SmartContractAddresses.cache[(address, transactionHash)] = !codeIsEmpty; + } return !codeIsEmpty; } @@ -219,7 +225,7 @@ export async function getTransactionActionKey(transaction) { return ret; } } - const toSmartContract = await isSmartContractAddress(to); + const toSmartContract = await isSmartContractAddress(to, transactionHash); if (toSmartContract) { // There is no data or unknown method data, if is smart contract ret = SMART_CONTRACT_INTERACTION_ACTION_KEY; diff --git a/app/util/transactions.test.js b/app/util/transactions.test.js index 19d184e6772..609b01fc31d 100644 --- a/app/util/transactions.test.js +++ b/app/util/transactions.test.js @@ -98,6 +98,28 @@ describe('Transactions utils :: isSmartContractAddress', () => { await isSmartContractAddress('0x89d24A6b4CcB1B6fAA2625fE562bDD9a23260359'); expect(stub).not.toBeCalled(); }); + + it('isSmartContractAddress should call query if not cached', async () => { + Engine.context = MOCK_ENGINE.context; + const stub = spyOn(Engine.context.TransactionController, 'query'); + await isSmartContractAddress('0x1', '0x123'); + expect(stub).toBeCalled(); + }); + + it('isSmartContractAddress should not call query if cached', async () => { + Engine.context = MOCK_ENGINE.context; + await isSmartContractAddress('0x1', '0x123'); + const stub = spyOn(Engine.context.TransactionController, 'query'); + await isSmartContractAddress('0x1', '0x123'); + expect(stub).not.toBeCalled(); + }); + + it('isSmartContractAddress should call query if only address was provided', async () => { + Engine.context = MOCK_ENGINE.context; + const stub = spyOn(Engine.context.TransactionController, 'query'); + await isSmartContractAddress('0x1'); + expect(stub).toBeCalled(); + }); }); describe('Transactions utils :: getTransactionActionKey', () => { @@ -109,21 +131,27 @@ describe('Transactions utils :: getTransactionActionKey', () => { const randomData = '0x987654321'; const transferFromData = '0x23b872dd0000000000000000000000000000'; it('getTransactionActionKey send ether', async () => { - const get = await getTransactionActionKey({ transactionHash: '0x1', transaction: {} }); + const get = await getTransactionActionKey({ transactionHash: '0x1', transaction: { to: '0x0' } }); expect(get).toEqual(SEND_ETHER_ACTION_KEY); }); it('getTransactionActionKey send ether with empty data', async () => { - const get = await getTransactionActionKey({ transactionHash: '0x1', transaction: { data: '0x' } }); + const get = await getTransactionActionKey({ transactionHash: '0x1', transaction: { data: '0x', to: '0x0' } }); expect(get).toEqual(SEND_ETHER_ACTION_KEY); }); it('getTransactionActionKey send token', async () => { - const get = await getTransactionActionKey({ transactionHash: '0x2', transaction: { data: transferData } }); + const get = await getTransactionActionKey({ + transactionHash: '0x2', + transaction: { data: transferData, to: '0x0' } + }); expect(get).toEqual(SEND_TOKEN_ACTION_KEY); }); it('getTransactionActionKey send collectible', async () => { - const get = await getTransactionActionKey({ transactionHash: '0x6', transaction: { data: transferFromData } }); + const get = await getTransactionActionKey({ + transactionHash: '0x6', + transaction: { data: transferFromData, to: '0x0' } + }); expect(get).toEqual(TRANSFER_FROM_ACTION_KEY); }); From 8053717d6215bad520ba7d1fdf6410ad03543489 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 26 Mar 2019 16:31:40 -0300 Subject: [PATCH 2/4] handle address code with gaba --- app/util/transactions.js | 31 ++++++------------------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/app/util/transactions.js b/app/util/transactions.js index d150f103e2c..13dc472f4a1 100644 --- a/app/util/transactions.js +++ b/app/util/transactions.js @@ -3,6 +3,7 @@ import { rawEncode, rawDecode } from 'ethereumjs-abi'; import Engine from '../core/Engine'; import { strings } from '../../locales/i18n'; import contractMap from 'eth-contract-metadata'; +import { isSmartContractCode } from 'gaba/util'; export const TOKEN_METHOD_TRANSFER = 'transfer'; export const TOKEN_METHOD_APPROVE = 'approve'; @@ -29,14 +30,6 @@ class ActionKeys { static cache = {}; } -/** - * Utility class with the single responsibility - * of caching SmartContractAddresses - */ -class SmartContractAddresses { - static cache = {}; -} - /** * Utility class with the single responsibility * of caching CollectibleAddresses @@ -140,31 +133,18 @@ export function getMethodData(data) { * Returns wether the given address is a contract * * @param {string} address - Ethereum address - * @param {string} transactionHash? - Transaction hash * @returns {boolean} - Wether the given address is a contract */ -export async function isSmartContractAddress(address, transactionHash) { +export async function isSmartContractAddress(address) { address = toChecksumAddress(address); // If in contract map we don't need to cache it if (contractMap[address]) { return Promise.resolve(true); } - // Look in cache memory only for specific time (transaction hash) - if (transactionHash) { - const cache = SmartContractAddresses.cache[(address, transactionHash)]; - if (cache) { - return Promise.resolve(cache); - } - } const { TransactionController } = Engine.context; const code = address ? await TransactionController.query('getCode', [address]) : undefined; - // Geth will return '0x', and ganache-core v2.2.1 will return '0x0' - const codeIsEmpty = !code || code === '0x' || code === '0x0'; - // Save in cache for specific time (transaction hash) - if (transactionHash) { - SmartContractAddresses.cache[(address, transactionHash)] = !codeIsEmpty; - } - return !codeIsEmpty; + const isSmartContract = isSmartContractCode(code); + return isSmartContract; } /** @@ -225,7 +205,8 @@ export async function getTransactionActionKey(transaction) { return ret; } } - const toSmartContract = await isSmartContractAddress(to, transactionHash); + const toSmartContract = + transaction.toSmartContract !== undefined ? transaction.toSmartContract : await isSmartContractAddress(to); if (toSmartContract) { // There is no data or unknown method data, if is smart contract ret = SMART_CONTRACT_INTERACTION_ACTION_KEY; From ad9362bd876318835cf266cdc1a443fc32082653 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Tue, 26 Mar 2019 16:33:54 -0300 Subject: [PATCH 3/4] remove cache test since we don't have anymore --- app/util/transactions.test.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/util/transactions.test.js b/app/util/transactions.test.js index 609b01fc31d..65aa654eced 100644 --- a/app/util/transactions.test.js +++ b/app/util/transactions.test.js @@ -106,14 +106,6 @@ describe('Transactions utils :: isSmartContractAddress', () => { expect(stub).toBeCalled(); }); - it('isSmartContractAddress should not call query if cached', async () => { - Engine.context = MOCK_ENGINE.context; - await isSmartContractAddress('0x1', '0x123'); - const stub = spyOn(Engine.context.TransactionController, 'query'); - await isSmartContractAddress('0x1', '0x123'); - expect(stub).not.toBeCalled(); - }); - it('isSmartContractAddress should call query if only address was provided', async () => { Engine.context = MOCK_ENGINE.context; const stub = spyOn(Engine.context.TransactionController, 'query'); From b98ca3f1a13c84e7e5719f20fa82e580f5fd67d5 Mon Sep 17 00:00:00 2001 From: Esteban Mino Date: Fri, 29 Mar 2019 19:43:40 -0300 Subject: [PATCH 4/4] bump gaba --- package-lock.json | 93 +++++++++++++++++++---------------------------- package.json | 2 +- 2 files changed, 38 insertions(+), 57 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7a55d1ba043..097d7568807 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4428,9 +4428,9 @@ } }, "caniuse-lite": { - "version": "1.0.30000950", - "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30000950.tgz", - "integrity": "sha512-Cs+4U9T0okW2ftBsCIHuEYXXkki7mjXmjCh4c6PzYShk04qDEr76/iC7KwhLoWoY65wcra1XOsRD+S7BptEb5A==" + "version": "1.0.30000955", + "resolved": "https://registry.npmjs.org/caniuse-lite/-/caniuse-lite-1.0.30000955.tgz", + "integrity": "sha512-6AwmIKgqCYfDWWadRkAuZSHMQP4Mmy96xAXEdRBlN/luQhlRYOKgwOlZ9plpCOsVbBuqbTmGqDK3JUM/nlr8CA==" }, "capture-exit": { "version": "1.2.0", @@ -5435,9 +5435,9 @@ "integrity": "sha1-WQxhFWsK4vTwJVcyoViyZrxWsh0=" }, "electron-to-chromium": { - "version": "1.3.116", - "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.3.116.tgz", - "integrity": "sha512-NKwKAXzur5vFCZYBHpdWjTMO8QptNLNP80nItkSIgUOapPAo9Uia+RvkCaZJtO7fhQaVElSvBPWEc2ku6cKsPA==" + "version": "1.3.122", + "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.3.122.tgz", + "integrity": "sha512-3RKoIyCN4DhP2dsmleuFvpJAIDOseWH88wFYBzb22CSwoFDSWRc4UAMfrtc9h8nBdJjTNIN3rogChgOy6eFInw==" }, "elegant-spinner": { "version": "1.0.1", @@ -6210,12 +6210,12 @@ "resolved": "https://registry.npmjs.org/eth-sig-util/-/eth-sig-util-1.4.2.tgz", "integrity": "sha1-jZWCAsftuq6Dlwf7pvCf8ydgYhA=", "requires": { - "ethereumjs-abi": "git+https://github.com/ethereumjs/ethereumjs-abi.git", + "ethereumjs-abi": "git+https://github.com/ethereumjs/ethereumjs-abi.git#572d4bafe08a8a231137e1f9daeb0f8a23f197d2", "ethereumjs-util": "^5.1.1" } }, "ethereumjs-abi": { - "version": "git+https://github.com/ethereumjs/ethereumjs-abi.git#d8d7fc9cc1fd781186c25676af100d1ec727013e", + "version": "git+https://github.com/ethereumjs/ethereumjs-abi.git#572d4bafe08a8a231137e1f9daeb0f8a23f197d2", "from": "git+https://github.com/ethereumjs/ethereumjs-abi.git", "requires": { "bn.js": "^4.11.8", @@ -6272,9 +6272,9 @@ } }, "eth-sig-util": { - "version": "2.1.1", - "resolved": "https://registry.npmjs.org/eth-sig-util/-/eth-sig-util-2.1.1.tgz", - "integrity": "sha512-B9VA2WCuf+dp0UbWlzsCXWcryZe1H9PixrNmG+tQDBpyTiIbDvf2w8jUb1BNPbxFXeWHUcr2I6pmg+MkdA4Ovg==", + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/eth-sig-util/-/eth-sig-util-2.1.2.tgz", + "integrity": "sha512-bNgt7txkEmaNlLf+PrbwYIdp4KRkB3E7hW0/QwlBpgv920pVVyQnF0evoovfiRveNKM4OrtPYZTojjmsfuCUOw==", "requires": { "buffer": "^5.2.1", "elliptic": "^6.4.0", @@ -7359,8 +7359,7 @@ }, "ansi-regex": { "version": "2.1.1", - "bundled": true, - "optional": true + "bundled": true }, "aproba": { "version": "1.2.0", @@ -7378,13 +7377,11 @@ }, "balanced-match": { "version": "1.0.0", - "bundled": true, - "optional": true + "bundled": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, - "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -7397,18 +7394,15 @@ }, "code-point-at": { "version": "1.1.0", - "bundled": true, - "optional": true + "bundled": true }, "concat-map": { "version": "0.0.1", - "bundled": true, - "optional": true + "bundled": true }, "console-control-strings": { "version": "1.1.0", - "bundled": true, - "optional": true + "bundled": true }, "core-util-is": { "version": "1.0.2", @@ -7511,8 +7505,7 @@ }, "inherits": { "version": "2.0.3", - "bundled": true, - "optional": true + "bundled": true }, "ini": { "version": "1.3.5", @@ -7522,7 +7515,6 @@ "is-fullwidth-code-point": { "version": "1.0.0", "bundled": true, - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -7535,20 +7527,17 @@ "minimatch": { "version": "3.0.4", "bundled": true, - "optional": true, "requires": { "brace-expansion": "^1.1.7" } }, "minimist": { "version": "0.0.8", - "bundled": true, - "optional": true + "bundled": true }, "minipass": { "version": "2.3.5", "bundled": true, - "optional": true, "requires": { "safe-buffer": "^5.1.2", "yallist": "^3.0.0" @@ -7565,7 +7554,6 @@ "mkdirp": { "version": "0.5.1", "bundled": true, - "optional": true, "requires": { "minimist": "0.0.8" } @@ -7638,8 +7626,7 @@ }, "number-is-nan": { "version": "1.0.1", - "bundled": true, - "optional": true + "bundled": true }, "object-assign": { "version": "4.1.1", @@ -7649,7 +7636,6 @@ "once": { "version": "1.4.0", "bundled": true, - "optional": true, "requires": { "wrappy": "1" } @@ -7725,8 +7711,7 @@ }, "safe-buffer": { "version": "5.1.2", - "bundled": true, - "optional": true + "bundled": true }, "safer-buffer": { "version": "2.1.2", @@ -7756,7 +7741,6 @@ "string-width": { "version": "1.0.2", "bundled": true, - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -7774,7 +7758,6 @@ "strip-ansi": { "version": "3.0.1", "bundled": true, - "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -7813,13 +7796,11 @@ }, "wrappy": { "version": "1.0.2", - "bundled": true, - "optional": true + "bundled": true }, "yallist": { "version": "3.0.3", - "bundled": true, - "optional": true + "bundled": true } } }, @@ -7893,12 +7874,12 @@ } }, "gaba": { - "version": "1.0.0-beta.64", - "resolved": "https://registry.npmjs.org/gaba/-/gaba-1.0.0-beta.64.tgz", - "integrity": "sha512-2eQrMdDxb4VBd7xWRwt9TMSgqUpIk+WcAf14ZsJNmLIJym74mbq1PXUx2/BFsf8s4rgENiqGd3IjTWVPFE7+DA==", + "version": "1.0.0-beta.67", + "resolved": "https://registry.npmjs.org/gaba/-/gaba-1.0.0-beta.67.tgz", + "integrity": "sha512-8CxuMriGvzhNdbeP8whkN6Il8Lx15kt4Dubh8nwHUOFl5+0LvNlsWG0iHh0wbcYiY6DkajX3Q4YnqTwxsQ51Sg==", "requires": { "await-semaphore": "^0.1.3", - "eth-contract-metadata": "github:estebanmino/eth-contract-metadata#e307359ca9ea6be4ded6ac58ac7e16f192ae4e2a", + "eth-contract-metadata": "github:MetaMask/eth-contract-metadata#faa4f56fb17b3ae8579df68708be59d617732f31", "eth-json-rpc-infura": "^3.1.2", "eth-keyring-controller": "^4.0.0", "eth-phishing-detect": "^1.1.13", @@ -7919,8 +7900,8 @@ }, "dependencies": { "eth-contract-metadata": { - "version": "github:estebanmino/eth-contract-metadata#e307359ca9ea6be4ded6ac58ac7e16f192ae4e2a", - "from": "github:estebanmino/eth-contract-metadata#e307359ca9ea6be4ded6ac58ac7e16f192ae4e2a" + "version": "github:MetaMask/eth-contract-metadata#faa4f56fb17b3ae8579df68708be59d617732f31", + "from": "github:MetaMask/eth-contract-metadata#faa4f56fb17b3ae8579df68708be59d617732f31" }, "ethereumjs-util": { "version": "5.2.0", @@ -17954,7 +17935,7 @@ "resolved": "https://registry.npmjs.org/web3/-/web3-0.20.7.tgz", "integrity": "sha512-VU6/DSUX93d1fCzBz7WP/SGCQizO1rKZi4Px9j/3yRyfssHyFcZamMw2/sj4E8TlfMXONvZLoforR8B4bRoyTQ==", "requires": { - "bignumber.js": "git+https://github.com/frozeman/bignumber.js-nolookahead.git", + "bignumber.js": "git+https://github.com/frozeman/bignumber.js-nolookahead.git#57692b3ecfc98bbdd6b3a516cb2353652ea49934", "crypto-js": "^3.1.4", "utf8": "^2.1.1", "xhr2-cookies": "^1.1.0", @@ -17998,26 +17979,26 @@ "resolved": "https://registry.npmjs.org/eth-sig-util/-/eth-sig-util-1.4.2.tgz", "integrity": "sha1-jZWCAsftuq6Dlwf7pvCf8ydgYhA=", "requires": { - "ethereumjs-abi": "git+https://github.com/ethereumjs/ethereumjs-abi.git", + "ethereumjs-abi": "git+https://github.com/ethereumjs/ethereumjs-abi.git#572d4bafe08a8a231137e1f9daeb0f8a23f197d2", "ethereumjs-util": "^5.1.1" } }, "ethereumjs-abi": { - "version": "git+https://github.com/ethereumjs/ethereumjs-abi.git#d84a96796079c8595a0c78accd1e7709f2277215", + "version": "git+https://github.com/ethereumjs/ethereumjs-abi.git#572d4bafe08a8a231137e1f9daeb0f8a23f197d2", "from": "git+https://github.com/ethereumjs/ethereumjs-abi.git", "requires": { - "bn.js": "^4.10.0", - "ethereumjs-util": "^5.0.0" + "bn.js": "^4.11.8", + "ethereumjs-util": "^6.0.0" }, "dependencies": { "ethereumjs-util": { - "version": "5.2.0", - "resolved": "https://registry.npmjs.org/ethereumjs-util/-/ethereumjs-util-5.2.0.tgz", - "integrity": "sha512-CJAKdI0wgMbQFLlLRtZKGcy/L6pzVRgelIZqRqNbuVFM3K9VEnyfbcvz0ncWMRNCe4kaHWjwRYQcYMucmwsnWA==", + "version": "6.1.0", + "resolved": "https://registry.npmjs.org/ethereumjs-util/-/ethereumjs-util-6.1.0.tgz", + "integrity": "sha512-URESKMFbDeJxnAxPppnk2fN6Y3BIatn9fwn76Lm8bQlt+s52TpG8dN9M66MLPuRAiAOIqL3dfwqWJf0sd0fL0Q==", "requires": { "bn.js": "^4.11.0", "create-hash": "^1.1.2", - "ethjs-util": "^0.1.3", + "ethjs-util": "0.1.6", "keccak": "^1.0.2", "rlp": "^2.0.0", "safe-buffer": "^5.1.1", diff --git a/package.json b/package.json index aad1c723aed..b631f3a6fed 100644 --- a/package.json +++ b/package.json @@ -70,7 +70,7 @@ "ethjs-unit": "0.1.6", "events": "3.0.0", "fuse.js": "3.4.4", - "gaba": "1.0.0-beta.64", + "gaba": "1.0.0-beta.67", "https-browserify": "0.0.1", "jsc-android": "236355.1.1", "multihashes": "0.4.14",