From 9fa6fc9d0525f5964d2fb98cf8432af9bcffc640 Mon Sep 17 00:00:00 2001 From: Daniel <80175477+dan437@users.noreply.github.com> Date: Fri, 23 Apr 2021 07:53:10 -0700 Subject: [PATCH] Add contract address validation for token swaps (#10912) --- shared/constants/network.js | 1 + shared/constants/swaps.js | 3 + test/jest/setup.js | 2 +- ui/app/ducks/swaps/swaps.js | 16 +++ ui/app/pages/swaps/swaps.util.js | 44 ++++++- ui/app/pages/swaps/swaps.util.test.js | 175 +++++++++++++++++++++++++- 6 files changed, 237 insertions(+), 4 deletions(-) diff --git a/shared/constants/network.js b/shared/constants/network.js index 6a71ad68a..a9bfa10ae 100644 --- a/shared/constants/network.js +++ b/shared/constants/network.js @@ -33,6 +33,7 @@ export const MAINNET_DISPLAY_NAME = 'Ethereum Mainnet'; export const GOERLI_DISPLAY_NAME = 'Goerli'; export const ETH_SYMBOL = 'ETH'; +export const WETH_SYMBOL = 'WETH'; export const TEST_ETH_SYMBOL = 'TESTETH'; export const BNB_SYMBOL = 'BNB'; diff --git a/shared/constants/swaps.js b/shared/constants/swaps.js index 259f13e2b..3f345542b 100644 --- a/shared/constants/swaps.js +++ b/shared/constants/swaps.js @@ -51,6 +51,9 @@ const TESTNET_CONTRACT_ADDRESS = '0x881d40237659c251811cec9c364ef91dc08d300c'; const BSC_CONTRACT_ADDRESS = '0x1a1ec25dc08e98e5e93f1104b5e5cdd298707d31'; +export const ETH_WETH_CONTRACT_ADDRESS = + '0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2'; + const METASWAP_ETH_API_HOST = 'https://api.metaswap.codefi.network'; const METASWAP_BSC_API_HOST = 'https://bsc-api.metaswap.codefi.network'; diff --git a/test/jest/setup.js b/test/jest/setup.js index bbdce03b8..6176cfc66 100644 --- a/test/jest/setup.js +++ b/test/jest/setup.js @@ -1,2 +1,2 @@ -// jest-setup.js is for Jest-specific setup only and runs before our Jest tests. +// This file is for Jest-specific setup only and runs before our Jest tests. import '@testing-library/jest-dom'; diff --git a/ui/app/ducks/swaps/swaps.js b/ui/app/ducks/swaps/swaps.js index 0e6f33bdb..f33b06745 100644 --- a/ui/app/ducks/swaps/swaps.js +++ b/ui/app/ducks/swaps/swaps.js @@ -2,6 +2,8 @@ import { createSlice } from '@reduxjs/toolkit'; import BigNumber from 'bignumber.js'; import log from 'loglevel'; +import { captureMessage } from '@sentry/browser'; + import { addToken, addUnapprovedTransaction, @@ -33,6 +35,7 @@ import { import { fetchSwapsFeatureLiveness, fetchSwapsGasPrices, + isContractAddressValid, } from '../../pages/swaps/swaps.util'; import { calcGasTotal } from '../../pages/send/send.utils'; import { @@ -676,6 +679,19 @@ export const signAndSendTransactions = (history, metaMetricsEvent) => { sensitiveProperties: swapMetaData, }); + if (!isContractAddressValid(usedTradeTxParams.to, swapMetaData, chainId)) { + captureMessage('Invalid contract address', { + extra: { + token_from: swapMetaData.token_from, + token_to: swapMetaData.token_to, + contract_address: usedTradeTxParams.to, + }, + }); + await dispatch(setSwapsErrorKey(SWAP_FAILED_ERROR)); + history.push(SWAPS_ERROR_ROUTE); + return; + } + let finalApproveTxMeta; const approveTxParams = getApproveTxParams(state); if (approveTxParams) { diff --git a/ui/app/pages/swaps/swaps.util.js b/ui/app/pages/swaps/swaps.util.js index 8606ea8d3..50b7d8666 100644 --- a/ui/app/pages/swaps/swaps.util.js +++ b/ui/app/pages/swaps/swaps.util.js @@ -5,13 +5,18 @@ import { isValidAddress } from 'ethereumjs-util'; import { SWAPS_CHAINID_DEFAULT_TOKEN_MAP, METASWAP_CHAINID_API_HOST_MAP, + SWAPS_CHAINID_CONTRACT_ADDRESS_MAP, + ETH_WETH_CONTRACT_ADDRESS, } from '../../../../shared/constants/swaps'; import { isSwapsDefaultTokenAddress, isSwapsDefaultTokenSymbol, } from '../../../../shared/modules/swaps.utils'; - -import { MAINNET_CHAIN_ID } from '../../../../shared/constants/network'; +import { + ETH_SYMBOL, + WETH_SYMBOL, + MAINNET_CHAIN_ID, +} from '../../../../shared/constants/network'; import { calcTokenValue, calcTokenAmount, @@ -676,3 +681,38 @@ export function formatSwapsValueForDisplay(destinationAmount) { } return amountToDisplay; } + +/** + * Checks whether a contract address is valid before swapping tokens. + * + * @param {string} contractAddress - E.g. "0x881d40237659c251811cec9c364ef91dc08d300c" for mainnet + * @param {object} swapMetaData - We check the following 2 fields, e.g. { token_from: "ETH", token_to: "WETH" } + * @param {string} chainId - The hex encoded chain ID to check + * @returns {boolean} Whether a contract address is valid or not + */ +export const isContractAddressValid = ( + contractAddress, + swapMetaData, + chainId = MAINNET_CHAIN_ID, +) => { + if (!contractAddress) { + return false; + } + if ( + (swapMetaData.token_from === ETH_SYMBOL && + swapMetaData.token_to === WETH_SYMBOL) || + (swapMetaData.token_from === WETH_SYMBOL && + swapMetaData.token_to === ETH_SYMBOL) + ) { + // Sometimes we get a contract address with a few upper-case chars and since addresses are + // case-insensitive, we compare uppercase versions for validity. + return ( + contractAddress.toUpperCase() === ETH_WETH_CONTRACT_ADDRESS.toUpperCase() + ); + } + const contractAddressForChainId = SWAPS_CHAINID_CONTRACT_ADDRESS_MAP[chainId]; + return ( + contractAddressForChainId && + contractAddressForChainId.toUpperCase() === contractAddress.toUpperCase() + ); +}; diff --git a/ui/app/pages/swaps/swaps.util.test.js b/ui/app/pages/swaps/swaps.util.test.js index 367ebbaa7..f955e3bae 100644 --- a/ui/app/pages/swaps/swaps.util.test.js +++ b/ui/app/pages/swaps/swaps.util.test.js @@ -1,5 +1,15 @@ import nock from 'nock'; -import { MAINNET_CHAIN_ID } from '../../../../shared/constants/network'; +import { + ETH_SYMBOL, + WETH_SYMBOL, + MAINNET_CHAIN_ID, + BSC_CHAIN_ID, + LOCALHOST_CHAIN_ID, +} from '../../../../shared/constants/network'; +import { + SWAPS_CHAINID_CONTRACT_ADDRESS_MAP, + ETH_WETH_CONTRACT_ADDRESS, +} from '../../../../shared/constants/swaps'; import { TOKENS, EXPECTED_TOKENS_RESULT, @@ -13,6 +23,7 @@ import { fetchTokens, fetchAggregatorMetadata, fetchTopAssets, + isContractAddressValid, } from './swaps.util'; jest.mock('../../../lib/storage-helpers.js', () => ({ @@ -165,4 +176,166 @@ describe('Swaps Util', () => { expect(result).toStrictEqual(expectedResult); }); }); + + describe('isContractAddressValid', () => { + let swapMetaData; + let usedTradeTxParams; + + beforeEach(() => { + swapMetaData = { + available_quotes: undefined, + average_savings: undefined, + best_quote_source: 'paraswap', + custom_slippage: true, + estimated_gas: '134629', + fee_savings: undefined, + gas_fees: '47.411896', + median_metamask_fee: undefined, + other_quote_selected: false, + other_quote_selected_source: '', + performance_savings: undefined, + slippage: 5, + suggested_gas_price: '164', + token_from: ETH_SYMBOL, + token_from_amount: '1', + token_to: WETH_SYMBOL, + token_to_amount: '1.0000000', + used_gas_price: '164', + }; + usedTradeTxParams = { + data: 'testData', + from: '0xe53a5bc256898bfa5673b20aceeb2b2152075d17', + gas: '2427c', + gasPrice: '27592f5a00', + to: ETH_WETH_CONTRACT_ADDRESS, + value: '0xde0b6b3a7640000', + }; + }); + + it('returns true if "token_from" is ETH, "token_to" is WETH and "to" is ETH_WETH contract address', () => { + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + MAINNET_CHAIN_ID, + ), + ).toBe(true); + }); + + it('returns true if "token_from" is WETH, "token_to" is ETH and "to" is ETH_WETH contract address', () => { + swapMetaData.token_from = WETH_SYMBOL; + swapMetaData.token_to = ETH_SYMBOL; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + MAINNET_CHAIN_ID, + ), + ).toBe(true); + }); + + it('returns true if "token_from" is ETH, "token_to" is WETH and "to" is ETH_WETH contract address with some uppercase chars', () => { + usedTradeTxParams.to = '0xc02AAA39B223fe8d0a0e5c4f27ead9083c756cc2'; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + MAINNET_CHAIN_ID, + ), + ).toBe(true); + }); + + it('returns false if "token_from" is ETH, "token_to" is WETH and "to" is mainnet contract address', () => { + usedTradeTxParams.to = + SWAPS_CHAINID_CONTRACT_ADDRESS_MAP[MAINNET_CHAIN_ID]; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + MAINNET_CHAIN_ID, + ), + ).toBe(false); + }); + + it('returns false if "token_from" is WETH, "token_to" is ETH and "to" is mainnet contract address', () => { + swapMetaData.token_from = WETH_SYMBOL; + swapMetaData.token_to = ETH_SYMBOL; + usedTradeTxParams.to = + SWAPS_CHAINID_CONTRACT_ADDRESS_MAP[MAINNET_CHAIN_ID]; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + MAINNET_CHAIN_ID, + ), + ).toBe(false); + }); + + it('returns false if contractAddress is null', () => { + expect( + isContractAddressValid(null, swapMetaData, LOCALHOST_CHAIN_ID), + ).toBe(false); + }); + + it('returns true if "token_from" is BAT and "to" is mainnet contract address', () => { + swapMetaData.token_from = 'BAT'; + usedTradeTxParams.to = + SWAPS_CHAINID_CONTRACT_ADDRESS_MAP[MAINNET_CHAIN_ID]; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + MAINNET_CHAIN_ID, + ), + ).toBe(true); + }); + + it('returns true if "token_to" is BAT and "to" is BSC contract address', () => { + swapMetaData.token_to = 'BAT'; + usedTradeTxParams.to = SWAPS_CHAINID_CONTRACT_ADDRESS_MAP[BSC_CHAIN_ID]; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + BSC_CHAIN_ID, + ), + ).toBe(true); + }); + + it('returns true if "token_to" is BAT and "to" is testnet contract address', () => { + swapMetaData.token_to = 'BAT'; + usedTradeTxParams.to = + SWAPS_CHAINID_CONTRACT_ADDRESS_MAP[LOCALHOST_CHAIN_ID]; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + LOCALHOST_CHAIN_ID, + ), + ).toBe(true); + }); + + it('returns true if "token_to" is BAT and "to" is testnet contract address with some uppercase chars', () => { + swapMetaData.token_to = 'BAT'; + usedTradeTxParams.to = '0x881D40237659C251811CEC9c364ef91dC08D300C'; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + LOCALHOST_CHAIN_ID, + ), + ).toBe(true); + }); + + it('returns false if "token_to" is BAT and "to" has mismatch with current chainId', () => { + swapMetaData.token_to = 'BAT'; + expect( + isContractAddressValid( + usedTradeTxParams.to, + swapMetaData, + LOCALHOST_CHAIN_ID, + ), + ).toBe(false); + }); + }); });