Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Mar 29, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89c1e0d
Fix 10517 - Prevent tokens without addresses from being added to toke…
darkwing Mar 17, 2021
de915da
Avoid touching hiddenTokens and accountHiddenTokens as they were not …
darkwing Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions app/scripts/migrations/056.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
},
};
155 changes: 155 additions & 0 deletions app/scripts/migrations/056.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
});
});
1 change: 1 addition & 0 deletions app/scripts/migrations/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const migrations = [
require('./053').default,
require('./054').default,
require('./055').default,
require('./056').default,
];

export default migrations;
2 changes: 2 additions & 0 deletions ui/app/ducks/swaps/swaps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ export const fetchQuotesAndSetQuoteState = (

let destinationTokenAddedForSwap = false;
if (
toTokenAddress &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because the next two lines would allow const selectedToToken = {} to add a token, which we definitely don't want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a fine change to protect against errors. However, we should never be in a situation where there is no toTokenAddress. If so, requesting quotes should fail.

In the issue description, it was described how editing the code so that toTokenAddress is undefined results in the described bug, which occurs after confirming the swap. So it sounds like requesting quotes succeeded (otherwise there wouldn't be a quote to submit on the view quote screen).

So that leads me to believe that there is some other value that is being used for the token address in the request for quote. Or swapsTokens contains a token that has an undefined address.

const destinationTokenInfo =
      swapsTokens?.find(({ address }) => address === toTokenAddress) ||
      selectedToToken;

It would be good if we could verify exactly how the request for quotes succeeds when const selectedToToken = {} and what the address is in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove the {} fallback from selectedFromToken and selectedToToken that existed from the initial swaps merge, and remove the toTokenAddress and fromTokenAddress checks in this PR.

If we should never get there without these properties, we should probably throw an error here, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we should never get there without these properties, we should probably throw an error here, right?

Right, but what I want to know is how was it possible to request quotes without these properties? Requesting quotes when from or two are empty strings or undefined would return 0 quotes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be addressed in another PR, as the change to actions.js handles the objectives of this PR.

toTokenSymbol !== swapsDefaultToken.symbol &&
!contractExchangeRates[toTokenAddress]
) {
Expand All @@ -440,6 +441,7 @@ export const fetchQuotesAndSetQuoteState = (
);
}
if (
fromTokenAddress &&
darkwing marked this conversation as resolved.
Show resolved Hide resolved
fromTokenSymbol !== swapsDefaultToken.symbol &&
!contractExchangeRates[fromTokenAddress] &&
fromTokenBalance &&
Expand Down
7 changes: 6 additions & 1 deletion ui/app/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1389,7 +1389,12 @@ export function addToken(
dontShowLoadingIndicator,
) {
return (dispatch) => {
!dontShowLoadingIndicator && dispatch(showLoadingIndication());
if (!address) {
darkwing marked this conversation as resolved.
Show resolved Hide resolved
throw new Error('MetaMask - Cannot add token without address');
darkwing marked this conversation as resolved.
Show resolved Hide resolved
}
if (!dontShowLoadingIndicator) {
dispatch(showLoadingIndication());
}
return new Promise((resolve, reject) => {
background.addToken(address, symbol, decimals, image, (err, tokens) => {
dispatch(hideLoadingIndication());
Expand Down
24 changes: 21 additions & 3 deletions ui/app/store/actions.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down Expand Up @@ -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);
});
Expand All @@ -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);
Expand Down