From 29cc31afe81159d0a21932a60ba5d3ce02b02985 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Mon, 29 Mar 2021 16:05:36 -0500 Subject: [PATCH] Fix 10517 - Prevent tokens without addresses from being added to token list (#10593) --- app/scripts/migrations/056.js | 51 ++++++++++ app/scripts/migrations/056.test.js | 155 +++++++++++++++++++++++++++++ app/scripts/migrations/index.js | 1 + ui/app/ducks/swaps/swaps.js | 2 + ui/app/store/actions.js | 7 +- ui/app/store/actions.test.js | 24 ++++- 6 files changed, 236 insertions(+), 4 deletions(-) create mode 100644 app/scripts/migrations/056.js create mode 100644 app/scripts/migrations/056.test.js diff --git a/app/scripts/migrations/056.js b/app/scripts/migrations/056.js new file mode 100644 index 000000000..f11d4b3f1 --- /dev/null +++ b/app/scripts/migrations/056.js @@ -0,0 +1,51 @@ +import { cloneDeep } from 'lodash'; + +const version = 56; + +/** + * Remove tokens that don't have an address due to + * lack of previous addToken validation. Also removes + * an unwanted, undefined image property + */ +export default { + version, + async migrate(originalVersionedData) { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + + const { PreferencesController } = versionedData.data; + + if (Array.isArray(PreferencesController.tokens)) { + PreferencesController.tokens = PreferencesController.tokens.filter( + ({ address }) => address, + ); + } + + if ( + PreferencesController.accountTokens && + typeof PreferencesController.accountTokens === 'object' + ) { + Object.keys(PreferencesController.accountTokens).forEach((account) => { + const chains = Object.keys( + PreferencesController.accountTokens[account], + ); + chains.forEach((chain) => { + PreferencesController.accountTokens[account][ + chain + ] = PreferencesController.accountTokens[account][chain].filter( + ({ address }) => address, + ); + }); + }); + } + + if ( + PreferencesController.assetImages && + 'undefined' in PreferencesController.assetImages + ) { + delete PreferencesController.assetImages.undefined; + } + + return versionedData; + }, +}; diff --git a/app/scripts/migrations/056.test.js b/app/scripts/migrations/056.test.js new file mode 100644 index 000000000..e311d4aee --- /dev/null +++ b/app/scripts/migrations/056.test.js @@ -0,0 +1,155 @@ +import assert from 'assert'; +import migration56 from './056'; + +const BAD_TOKEN_DATA = { symbol: null, decimals: null }; +const TOKEN2 = { symbol: 'TXT', address: '0x11', decimals: 18 }; +const TOKEN3 = { symbol: 'TVT', address: '0x12', decimals: 18 }; + +describe('migration #56', function () { + it('should update the version metadata', async function () { + const oldStorage = { + meta: { + version: 55, + }, + data: { + PreferencesController: { + tokens: [], + accountTokens: {}, + assetImages: {}, + }, + }, + }; + + const newStorage = await migration56.migrate(oldStorage); + assert.deepStrictEqual(newStorage.meta, { + version: 56, + }); + }); + + it(`should filter out tokens without a valid address property`, async function () { + const oldStorage = { + meta: {}, + data: { + PreferencesController: { + tokens: [BAD_TOKEN_DATA, TOKEN2, BAD_TOKEN_DATA, TOKEN3], + accountTokens: {}, + assetImages: {}, + }, + }, + }; + + const newStorage = await migration56.migrate(oldStorage); + assert.deepStrictEqual(newStorage.data.PreferencesController.tokens, [ + TOKEN2, + TOKEN3, + ]); + }); + + it(`should not filter any tokens when all token information is valid`, async function () { + const oldStorage = { + meta: {}, + data: { + PreferencesController: { + tokens: [TOKEN2, TOKEN3], + accountTokens: {}, + assetImages: {}, + }, + }, + }; + + const newStorage = await migration56.migrate(oldStorage); + assert.deepStrictEqual(newStorage.data.PreferencesController.tokens, [ + TOKEN2, + TOKEN3, + ]); + }); + + it(`should filter out accountTokens without a valid address property`, async function () { + const originalAccountTokens = { + '0x1111111111111111111111111': { + '0x1': [TOKEN2, TOKEN3, BAD_TOKEN_DATA], + '0x3': [], + '0x4': [BAD_TOKEN_DATA, BAD_TOKEN_DATA], + }, + '0x1111111111111111111111112': { + '0x1': [TOKEN2], + '0x3': [], + '0x4': [BAD_TOKEN_DATA, BAD_TOKEN_DATA], + }, + }; + + const oldStorage = { + meta: {}, + data: { + PreferencesController: { + tokens: [], + accountTokens: originalAccountTokens, + assetImages: {}, + }, + }, + }; + + const newStorage = await migration56.migrate(oldStorage); + + const desiredResult = { ...originalAccountTokens }; + // The last item in the array was bad and should be removed + desiredResult['0x1111111111111111111111111']['0x1'].pop(); + // All items in 0x4 were bad + desiredResult['0x1111111111111111111111111']['0x4'] = []; + desiredResult['0x1111111111111111111111112']['0x4'] = []; + + assert.deepStrictEqual( + newStorage.data.PreferencesController.accountTokens, + desiredResult, + ); + }); + + it(`should remove a bad assetImages key`, async function () { + const desiredAssetImages = { + '0x514910771af9ca656af840dff83e8264ecf986ca': + 'images/contract/chainlink.svg', + }; + const oldStorage = { + meta: {}, + data: { + PreferencesController: { + tokens: [], + accountTokens: {}, + assetImages: { ...desiredAssetImages, undefined: null }, + }, + }, + }; + + const newStorage = await migration56.migrate(oldStorage); + assert.deepStrictEqual( + newStorage.data.PreferencesController.assetImages, + desiredAssetImages, + ); + }); + + it(`token data with no problems should preserve all data`, async function () { + const perfectData = { + tokens: [TOKEN2, TOKEN3], + accountTokens: { + '0x1111111111111111111111111': { + '0x1': [], + '0x3': [TOKEN2], + }, + '0x1111111111111111111111112': { + '0x1': [TOKEN2, TOKEN3], + '0x3': [], + }, + }, + }; + + const oldStorage = { + meta: {}, + data: { + PreferencesController: perfectData, + }, + }; + + const newStorage = await migration56.migrate(oldStorage); + assert.deepStrictEqual(newStorage.data.PreferencesController, perfectData); + }); +}); diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index e03e4bc58..b26c3eacc 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -60,6 +60,7 @@ const migrations = [ require('./053').default, require('./054').default, require('./055').default, + require('./056').default, ]; export default migrations; diff --git a/ui/app/ducks/swaps/swaps.js b/ui/app/ducks/swaps/swaps.js index 64e78c125..1040c358e 100644 --- a/ui/app/ducks/swaps/swaps.js +++ b/ui/app/ducks/swaps/swaps.js @@ -417,6 +417,7 @@ export const fetchQuotesAndSetQuoteState = ( let destinationTokenAddedForSwap = false; if ( + toTokenAddress && toTokenSymbol !== swapsDefaultToken.symbol && !contractExchangeRates[toTokenAddress] ) { @@ -432,6 +433,7 @@ export const fetchQuotesAndSetQuoteState = ( ); } if ( + fromTokenAddress && fromTokenSymbol !== swapsDefaultToken.symbol && !contractExchangeRates[fromTokenAddress] && fromTokenBalance && diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 907f6fa10..10b07edfb 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -1389,7 +1389,12 @@ export function addToken( dontShowLoadingIndicator, ) { return (dispatch) => { - !dontShowLoadingIndicator && dispatch(showLoadingIndication()); + if (!address) { + throw new Error('MetaMask - Cannot add token without address'); + } + if (!dontShowLoadingIndicator) { + dispatch(showLoadingIndication()); + } return new Promise((resolve, reject) => { background.addToken(address, symbol, decimals, image, (err, tokens) => { dispatch(hideLoadingIndication()); diff --git a/ui/app/store/actions.test.js b/ui/app/store/actions.test.js index 52d7c6417..024cb0dc4 100644 --- a/ui/app/store/actions.test.js +++ b/ui/app/store/actions.test.js @@ -1177,7 +1177,13 @@ describe('Actions', function () { actions._setBackgroundConnection(background.getApi()); - await store.dispatch(actions.addToken()); + await store.dispatch( + actions.addToken({ + address: '0x514910771af9ca656af840dff83e8264ecf986ca', + symbol: 'LINK', + decimals: 18, + }), + ); assert(addTokenStub.calledOnce); }); @@ -1209,7 +1215,13 @@ describe('Actions', function () { }, ]; - await store.dispatch(actions.addToken()); + await store.dispatch( + actions.addToken({ + address: '0x514910771af9ca656af840dff83e8264ecf986ca', + symbol: 'LINK', + decimals: 18, + }), + ); assert.deepStrictEqual(store.getActions(), expectedActions); }); @@ -1234,7 +1246,13 @@ describe('Actions', function () { ]; try { - await store.dispatch(actions.addToken()); + await store.dispatch( + actions.addToken({ + address: '_', + symbol: '', + decimals: 0, + }), + ); assert.fail('Should have thrown error'); } catch (_) { assert.deepEqual(store.getActions(), expectedActions);