Fix 10517 - Prevent tokens without addresses from being added to token list (#10593)

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;
+  },
+};

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);
+  });
+});

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;

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 &&

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());

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);