From 6efe98f38ac535f51dacba7b794545c9d0a05422 Mon Sep 17 00:00:00 2001 From: Samuel Holmes Date: Thu, 26 Aug 2021 14:32:03 -0700 Subject: [PATCH 1/2] Fix EIP-681 parsing to accept URIs with unknown contract addresses Previous implementation was too conservative with security when handling unknown tokens. Rather than be strict, we only throw an error when currency code or contract address is known and doesn't either the currency code or contract address supplied to `parseUri`. --- src/ethereum/ethPlugin.js | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/ethereum/ethPlugin.js b/src/ethereum/ethPlugin.js index 0c01ed9e8..519947e00 100644 --- a/src/ethereum/ethPlugin.js +++ b/src/ethereum/ethPlugin.js @@ -239,18 +239,30 @@ export class EthereumPlugin extends CurrencyPlugin { ? biggyScience(parameters.uint256) : edgeParsedUri.nativeAmount - // Get meta token from contract address - const metaToken = this.currencyInfo.metaTokens.find( - metaToken => metaToken.contractAddress === contractAddress + // We know the token which is being used by either the URI contract + // address or the given currency code + const knownMetaToken = this.currencyInfo.metaTokens.find( + metaToken => + metaToken.contractAddress === contractAddress || + metaToken.currencyCode === currencyCode ) - // If there is a currencyCode param, the metaToken must be found - // and it's currency code must matching the currencyCode param. + // Don't allow transfers to incorrect contract addresses. + // It would clearly be a user error to transfer some amount from a + // contract address that doesn't match the given currency code. if ( currencyCode != null && - (metaToken == null || metaToken.currencyCode !== currencyCode) + knownMetaToken != null && + (knownMetaToken.currencyCode !== currencyCode || + knownMetaToken.contractAddress !== contractAddress) ) { - throw new Error('InternalErrorInvalidCurrencyCode') + const { + currencyCode: knownCurrencyCode, + contractAddress: knownContractAddress = '' + } = knownMetaToken + throw new Error( + `Currency code ${currencyCode} and contract ${contractAddress} must match known currency code ${knownCurrencyCode} and contract ${knownContractAddress}` + ) } // Validate addresses @@ -263,7 +275,7 @@ export class EthereumPlugin extends CurrencyPlugin { return { ...edgeParsedUri, - currencyCode: metaToken?.currencyCode, + currencyCode: knownMetaToken?.currencyCode, nativeAmount, publicAddress } From 8b5dbddf564d5a37333d7b948c14d9370e894a20 Mon Sep 17 00:00:00 2001 From: Samuel Holmes Date: Thu, 26 Aug 2021 14:53:21 -0700 Subject: [PATCH 2/2] Improve EIP-681 unit test case coverage --- test/plugin/fixtures.js | 68 +++++++++++++++++++++++++++++++------- test/plugin/plugin.test.js | 42 ++++++++++++----------- 2 files changed, 79 insertions(+), 31 deletions(-) diff --git a/test/plugin/fixtures.js b/test/plugin/fixtures.js index bc9a176fc..e32eeef30 100644 --- a/test/plugin/fixtures.js +++ b/test/plugin/fixtures.js @@ -379,12 +379,6 @@ export default [ '0x04b6b3bcbc16a5fb6a20301d650f8def513122a8', '0x04b6b3bcbc16a5fb6a20301d650f8def513122a8' ], - 'address with provided currency code': { - args: ['0x04b6b3bcbc16a5fb6a20301d650f8def513122a8', 'USDC'], - output: { - publicAddress: '0x04b6b3bcbc16a5fb6a20301d650f8def513122a8' - } - }, 'checksum address only': [ '0x3C40cbb7F82A7E1bc83C4E3E98590b19e0e1bf07', '0x3c40cbb7f82a7e1bc83c4e3e98590b19e0e1bf07' @@ -437,15 +431,34 @@ export default [ '0x04b6b3bcbc16a5fb6a20301d650f8def513122a8', '12345678900000000000000', 'ETH' - ], - 'uri eip681 payment address': { + ] + }, + // Expected output given some arguments as input + parseUriArgCases: [ + { + name: 'Address with known currency code provided', + args: ['0x04b6b3bcbc16a5fb6a20301d650f8def513122a8', 'USDC'], + output: { + publicAddress: '0x04b6b3bcbc16a5fb6a20301d650f8def513122a8' + } + }, + { + name: 'Address with unknown currency code provided', + args: ['0x04b6b3bcbc16a5fb6a20301d650f8def513122a8', 'ABCDEF'], + output: { + publicAddress: '0x04b6b3bcbc16a5fb6a20301d650f8def513122a8' + } + }, + { + name: 'URI EIP-681 payment address', args: ['ethereum:0xf5d81254c269a1e984044e4d542adc07bf18c541?value=123'], output: { publicAddress: '0xf5d81254c269a1e984044e4d542adc07bf18c541', nativeAmount: '123' } }, - 'uri eip681 payment address with pay prefix': { + { + name: 'URI EIP-681 payment address with pay prefix', args: [ 'ethereum:pay-0xf5d81254c269a1e984044e4d542adc07bf18c541?value=123' ], @@ -454,7 +467,8 @@ export default [ nativeAmount: '123' } }, - 'uri eip681 payment address using scientific notation': { + { + name: 'URI EIP-681 payment address using scientific notation', args: [ 'ethereum:0xf5d81254c269a1e984044e4d542adc07bf18c541?value=2.014e18' ], @@ -463,7 +477,8 @@ export default [ nativeAmount: '2014000000000000000' } }, - 'uri eip681 transfer contract invocation': { + { + name: 'URI EIP-681 transfer of known currency code and contract address', args: [ 'ethereum:0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48/transfer?address=0xf5d81254c269a1e984044e4d542adc07bf18c541&uint256=2.014e6', 'USDC' @@ -473,8 +488,37 @@ export default [ nativeAmount: '2014000', currencyCode: 'USDC' } + }, + { + name: 'URI EIP-681 transfer of known contract address yet incorrect currency code', + args: [ + 'ethereum:0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48/transfer?address=0xf5d81254c269a1e984044e4d542adc07bf18c541&uint256=2.014e6', + 'ABCDEF' + ], + error: + 'Currency code ABCDEF and contract 0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48 must match known currency code USDC and contract 0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48' + }, + { + name: 'URI EIP-681 transfer of known currency code yet incorrect contract address', + args: [ + 'ethereum:0xabcdef0000000000000000000000000000000000/transfer?address=0xf5d81254c269a1e984044e4d542adc07bf18c541&uint256=2.014e6', + 'USDC' + ], + error: + 'Currency code USDC and contract 0xabcdef0000000000000000000000000000000000 must match known currency code USDC and contract 0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48' + }, + { + name: 'URI EIP-681 transfer of unknown currency code and contract address', + args: [ + 'ethereum:0xabcdef0000000000000000000000000000000000/transfer?address=0xf5d81254c269a1e984044e4d542adc07bf18c541&uint256=2.014e6', + 'ABCDEF' + ], + output: { + publicAddress: '0xf5d81254c269a1e984044e4d542adc07bf18c541', + nativeAmount: '2014000' + } } - }, + ], encodeUri: { 'address only': [ { publicAddress: '0x04b6b3bcbc16a5fb6a20301d650f8def513122a8' }, diff --git a/test/plugin/plugin.test.js b/test/plugin/plugin.test.js index c857e47e9..1432c3deb 100644 --- a/test/plugin/plugin.test.js +++ b/test/plugin/plugin.test.js @@ -260,33 +260,37 @@ for (const fixture of fixtures) { /* interface TestCase { - args: any[], - output: { - [key: string]: any; + name: string + args: any[] + output?: { + [key: string]: any } + error?: string } */ - ;[ - 'address only with provided currency code', - 'uri eip681 payment address', - 'uri eip681 payment address with pay prefix', - 'uri eip681 payment address using scientific notation', - 'uri eip681 transfer contract invocation' - ].forEach(function (caseName) { - const caseFixtures = fixture.parseUri[caseName] + if (fixture.parseUriArgCases && fixture.parseUriArgCases.length) { + fixture.parseUriArgCases.forEach(function (testCase) { + const { name: caseName, args, output, error } = testCase - if (caseFixtures == null) return + it(caseName, async function () { + await tools.parseUri(...args).then( + parsedUri => { + if (error != null) assert.fail('Reject expected: ' + error) - it(caseName, async function () { - const parsedUri = await tools.parseUri(...caseFixtures.args) + Object.entries(output).forEach(([key, value]) => { + assert.equal(parsedUri[key], value) + }) + }, + err => { + if (output != null) + assert.fail('Reject unexpected:' + err.message) - Object.entries(caseFixtures.output).forEach(([key, value]) => { - if (caseName === 'address only with provided currency code') - console.log(';;', parsedUri) - assert.equal(parsedUri[key], value) + assert.equal(error, err.message) + } + ) }) }) - }) + } }) describe(`encodeUri for Wallet type ${WALLET_TYPE}`, function () {