diff --git a/packages/notification-services-controller/package.json b/packages/notification-services-controller/package.json index 18c491858d..3f4f7d7bdc 100644 --- a/packages/notification-services-controller/package.json +++ b/packages/notification-services-controller/package.json @@ -102,7 +102,7 @@ "@contentful/rich-text-html-renderer": "^16.5.2", "@metamask/base-controller": "^7.0.1", "@metamask/controller-utils": "^11.3.0", - "bignumber.js": "^4.1.0", + "bignumber.js": "^9.1.2", "firebase": "^10.11.0", "loglevel": "^1.8.1", "uuid": "^8.3.2" diff --git a/packages/transaction-controller/package.json b/packages/transaction-controller/package.json index 3c34f8c3c1..e261585e00 100644 --- a/packages/transaction-controller/package.json +++ b/packages/transaction-controller/package.json @@ -61,6 +61,7 @@ "@metamask/rpc-errors": "^6.3.1", "@metamask/utils": "^9.1.0", "async-mutex": "^0.5.0", + "bignumber.js": "^9.1.2", "bn.js": "^5.2.1", "eth-method-registry": "^4.0.0", "fast-json-patch": "^3.1.1", diff --git a/packages/transaction-controller/src/TransactionController.test.ts b/packages/transaction-controller/src/TransactionController.test.ts index 030703f8cb..5dc758f9b0 100644 --- a/packages/transaction-controller/src/TransactionController.test.ts +++ b/packages/transaction-controller/src/TransactionController.test.ts @@ -36,6 +36,7 @@ import { errorCodes, providerErrors, rpcErrors } from '@metamask/rpc-errors'; import type { Hex } from '@metamask/utils'; import { createDeferredPromise } from '@metamask/utils'; import assert from 'assert'; +import { merge } from 'lodash'; import * as uuidModule from 'uuid'; import { FakeBlockTracker } from '../../../tests/fake-block-tracker'; @@ -73,6 +74,7 @@ import type { SubmitHistoryEntry, } from './types'; import { + BLOCKAID_RESULT_TYPE_MALICIOUS, GasFeeEstimateType, SimulationErrorCode, SimulationTokenStandard, @@ -5030,6 +5032,230 @@ describe('TransactionController', () => { 'Cannot update security alert response as no transaction metadata found', ); }); + + describe('should not trigger simulation data update', () => { + it('when simulationData is not available', async () => { + const transactionMeta = TRANSACTION_META_MOCK; + + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMeta], + }, + }, + }); + + controller.updateSecurityAlertResponse(transactionMeta.id, { + reason: 'NA', + // This is API specific hence naming convention is not followed. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: BLOCKAID_RESULT_TYPE_MALICIOUS, + }); + + expect(getSimulationDataMock).toHaveBeenCalledTimes(0); + }); + + it('when security alert response type is not malicious', async () => { + const transactionMetaWithSimulationData = merge( + {}, + TRANSACTION_META_MOCK, + { + txParams: { + value: '0x0', + }, + simulationData: { + tokenBalanceChanges: [], + nativeBalanceChange: { + difference: '0x0', + }, + }, + }, + ); + + getSimulationDataMock.mockResolvedValueOnce(SIMULATION_DATA_MOCK); + + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMetaWithSimulationData], + }, + }, + }); + + controller.updateSecurityAlertResponse( + transactionMetaWithSimulationData.id, + { + reason: 'NA', + // This is API specific hence naming convention is not followed. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: 'warning', + }, + ); + + expect(getSimulationDataMock).toHaveBeenCalledTimes(0); + }); + + it('when transaction value and simulated native balance change within threshold', async () => { + const transactionMetaWithSimulationData = merge( + {}, + TRANSACTION_META_MOCK, + { + txParams: { + value: '0x12345678', + }, + simulationData: { + tokenBalanceChanges: [], + nativeBalanceChange: { + difference: '0x12345178', + }, + }, + }, + ); + + getSimulationDataMock.mockResolvedValueOnce(SIMULATION_DATA_MOCK); + + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMetaWithSimulationData], + }, + }, + }); + + controller.updateSecurityAlertResponse( + transactionMetaWithSimulationData.id, + { + reason: 'NA', + // This is API specific hence naming convention is not followed. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: BLOCKAID_RESULT_TYPE_MALICIOUS, + }, + ); + + expect(getSimulationDataMock).toHaveBeenCalledTimes(0); + }); + }); + + it('should retrigger simulation data update when result_type is malicious and simulated transaction value above threshold', async () => { + const transactionMetaWithSimulationData = merge( + {}, + TRANSACTION_META_MOCK, + { + txParams: { + value: '0x012', + }, + simulationData: { + tokenBalanceChanges: [], + nativeBalanceChange: { + difference: '0x012345', + }, + }, + }, + ); + + getSimulationDataMock.mockResolvedValueOnce(SIMULATION_DATA_MOCK); + + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMetaWithSimulationData], + }, + }, + }); + + controller.updateSecurityAlertResponse( + transactionMetaWithSimulationData.id, + { + reason: 'NA', + // This is API specific hence naming convention is not followed. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: BLOCKAID_RESULT_TYPE_MALICIOUS, + }, + ); + + await flushPromises(); + + expect(getSimulationDataMock).toHaveBeenCalledTimes(1); + }); + + describe('should mark simulationDataChanged after security alert update', () => { + it('as true if simulationData updated', async () => { + // Assume that previous simulationData tells that transaction has no token balance changes + const transactionMetaWithSimulationData = merge( + {}, + TRANSACTION_META_MOCK, + { + txParams: { + value: '0x0', + }, + simulationData: { + tokenBalanceChanges: [], + nativeBalanceChange: { + difference: '0x012345', + }, + }, + }, + ); + + getSimulationDataMock.mockResolvedValueOnce(SIMULATION_DATA_MOCK); + + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMetaWithSimulationData], + }, + }, + }); + + controller.updateSecurityAlertResponse( + transactionMetaWithSimulationData.id, + { + reason: 'NA', + // This is API specific hence naming convention is not followed. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: BLOCKAID_RESULT_TYPE_MALICIOUS, + }, + ); + + await flushPromises(); + + expect( + controller.state.transactions[0].simulationData + ?.isReSimulatedDueToSecurityAlert, + ).toBe(true); + }); + + it('as undefined if current and new simulationData equality check is false', async () => { + // Assume that previous simulationData tells that transaction has some token balance changes + const transactionMeta = merge({}, TRANSACTION_META_MOCK, { + simulationData: SIMULATION_DATA_MOCK, + }); + + getSimulationDataMock.mockResolvedValueOnce(SIMULATION_DATA_MOCK); + + const { controller } = setupController({ + options: { + state: { + transactions: [transactionMeta], + }, + }, + }); + + controller.updateSecurityAlertResponse(transactionMeta.id, { + reason: 'NA', + // This is API specific hence naming convention is not followed. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: BLOCKAID_RESULT_TYPE_MALICIOUS, + }); + + await flushPromises(); + + expect( + controller.state.transactions[0].simulationData + ?.isReSimulatedDueToSecurityAlert, + ).toBeUndefined(); + }); + }); }); describe('updateCustodialTransaction', () => { diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index eec7192821..8a7de21714 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -84,6 +84,7 @@ import type { SubmitHistoryEntry, } from './types'; import { + BLOCKAID_RESULT_TYPE_MALICIOUS, TransactionEnvelopeType, TransactionType, TransactionStatus, @@ -115,6 +116,7 @@ import { determineTransactionType } from './utils/transaction-type'; import { normalizeTransactionParams, isEIP1559Transaction, + isPercentageDifferenceWithinThreshold, validateGasValues, validateIfTransactionUnapproved, normalizeTxError, @@ -3554,7 +3556,8 @@ export class TransactionController extends BaseController< }, callback: (transactionMeta: TransactionMeta) => TransactionMeta | void, ): Readonly { - let updatedTransactionParams: (keyof TransactionParams)[] = []; + let isReSimulateNeedDueToTxParamsUpdated = false; + let isReSimulateNeedDueToSecurityAlert = false; this.update((state) => { const index = state.transactions.findIndex( @@ -3574,8 +3577,10 @@ export class TransactionController extends BaseController< validateTxParams(transactionMeta.txParams); } - updatedTransactionParams = - this.#checkIfTransactionParamsUpdated(transactionMeta); + isReSimulateNeedDueToTxParamsUpdated = + this.#shouldReSimulateDueToTxParamUpdate(transactionMeta); + isReSimulateNeedDueToSecurityAlert = + this.#shouldReSimulateDueToSecurityAlert(transactionMeta); const shouldSkipHistory = this.isHistoryDisabled || skipHistory; @@ -3592,23 +3597,101 @@ export class TransactionController extends BaseController< transactionId, ) as TransactionMeta; - if (updatedTransactionParams.length > 0) { - this.#onTransactionParamsUpdated( - transactionMeta, - updatedTransactionParams, - ); + if ( + isReSimulateNeedDueToTxParamsUpdated || + isReSimulateNeedDueToSecurityAlert + ) { + this.#updateSimulationData(transactionMeta, { + isReSimulatedDueToSecurityAlert: isReSimulateNeedDueToSecurityAlert, + }).catch((error) => { + log('Error updating simulation data', error); + throw error; + }); } return transactionMeta; } - #checkIfTransactionParamsUpdated(newTransactionMeta: TransactionMeta) { + #shouldReSimulateDueToSecurityAlert(transactionMeta: TransactionMeta) { + const isSecurityAlertOrSimulationUpdated = + this.#isSecurityAlertOrSimulationUpdated(transactionMeta); + const { + securityAlertResponse, + simulationData, + txParams: { value }, + } = transactionMeta; + + const isSimulationDataAvailable = Boolean(simulationData); + const isMaliciousTransfer = + securityAlertResponse?.result_type === BLOCKAID_RESULT_TYPE_MALICIOUS; + const simulationNativeBalanceDifference = + simulationData?.nativeBalanceChange?.difference; + const isTxValueAndSimulationNativeBalanceDefined = + value !== undefined && simulationNativeBalanceDifference !== undefined; + const isTxValueVsBalanceAbovePercentageThreshold = + isTxValueAndSimulationNativeBalanceDefined && + !isPercentageDifferenceWithinThreshold( + simulationNativeBalanceDifference, + value as Hex, + 5, + ); + + return ( + isSecurityAlertOrSimulationUpdated && + isSimulationDataAvailable && + isMaliciousTransfer && + isTxValueVsBalanceAbovePercentageThreshold + ); + } + + #isSecurityAlertOrSimulationUpdated(newTransactionMeta: TransactionMeta) { + const { + id: transactionId, + simulationData: newSimulationData, + securityAlertResponse: newSecurityAlertResponse, + } = cloneDeep(newTransactionMeta); + + const newTransactionProps = { + simulationData: newSimulationData, + securityAlertResponse: newSecurityAlertResponse, + simulationNativeBalanceDifference: + newSimulationData?.nativeBalanceChange?.difference, + }; + + const { + simulationData: originalSimulationData, + securityAlertResponse: originalSecurityAlertResponse, + } = cloneDeep(this.getTransaction(transactionId) as TransactionMeta); + + const originalTransactionProps = { + simulationData: originalSimulationData, + securityAlertResponse: originalSecurityAlertResponse, + simulationNativeBalanceDifference: + originalSimulationData?.nativeBalanceChange?.difference, + }; + + // Skips the cases where simulationData fetch started or completed + const shouldSkip = + !newTransactionProps.simulationData || + (!originalTransactionProps.simulationData && + newTransactionProps.simulationData); + + if (shouldSkip) { + return false; + } + + return !isEqual(originalTransactionProps, newTransactionProps); + } + + #shouldReSimulateDueToTxParamUpdate( + newTransactionMeta: TransactionMeta, + ): boolean { const { id: transactionId, txParams: newParams } = newTransactionMeta; const originalParams = this.getTransaction(transactionId)?.txParams; if (!originalParams || isEqual(originalParams, newParams)) { - return []; + return false; } const params = Object.keys(newParams) as (keyof TransactionParams)[]; @@ -3625,31 +3708,27 @@ export class TransactionController extends BaseController< newParams, ); - return updatedProperties; - } - - #onTransactionParamsUpdated( - transactionMeta: TransactionMeta, - updatedParams: (keyof TransactionParams)[], - ) { - if ( - (['to', 'value', 'data'] as const).some((param) => - updatedParams.includes(param), - ) - ) { - log('Updating simulation data due to transaction parameter update'); - this.#updateSimulationData(transactionMeta).catch((error) => { - log('Error updating simulation data', error); - throw error; - }); - } + return (['to', 'value', 'data'] as const).some((param) => + updatedProperties.includes(param), + ); } async #updateSimulationData( transactionMeta: TransactionMeta, - { traceContext }: { traceContext?: TraceContext } = {}, + { + traceContext, + isReSimulatedDueToSecurityAlert, + }: { + traceContext?: TraceContext; + isReSimulatedDueToSecurityAlert?: boolean; + } = {}, ) { - const { id: transactionId, chainId, txParams } = transactionMeta; + const { + id: transactionId, + chainId, + txParams, + simulationData: prevSimulationData, + } = transactionMeta; const { from, to, value, data } = txParams; let simulationData: SimulationData = { @@ -3679,6 +3758,13 @@ export class TransactionController extends BaseController< data: data as Hex, }), ); + + if (prevSimulationData && !isEqual(simulationData, prevSimulationData)) { + simulationData = { + ...simulationData, + isReSimulatedDueToSecurityAlert, + }; + } } const finalTransactionMeta = this.getTransaction(transactionId); diff --git a/packages/transaction-controller/src/types.ts b/packages/transaction-controller/src/types.ts index d013817be0..6272d811b4 100644 --- a/packages/transaction-controller/src/types.ts +++ b/packages/transaction-controller/src/types.ts @@ -1119,7 +1119,7 @@ export type TransactionError = { export type SecurityAlertResponse = { reason: string; features?: string[]; - // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // This is API specific hence naming convention is not followed. // eslint-disable-next-line @typescript-eslint/naming-convention result_type: string; providerRequestsCount?: Record; @@ -1307,6 +1307,9 @@ export type SimulationError = { /** Simulation data for a transaction. */ export type SimulationData = { + /** Set to true if simulationData changed after updating security alert */ + isReSimulatedDueToSecurityAlert?: boolean; + /** Error data if the simulation failed or the transaction reverted. */ error?: SimulationError; @@ -1368,3 +1371,5 @@ export type SubmitHistoryEntry = { /** The transaction parameters that were submitted. */ transaction: TransactionParams; }; + +export const BLOCKAID_RESULT_TYPE_MALICIOUS = 'Malicious'; diff --git a/packages/transaction-controller/src/utils/utils.ts b/packages/transaction-controller/src/utils/utils.ts index 4c0a633fae..32ca8df753 100644 --- a/packages/transaction-controller/src/utils/utils.ts +++ b/packages/transaction-controller/src/utils/utils.ts @@ -3,7 +3,8 @@ import { getKnownPropertyNames, isStrictHexString, } from '@metamask/utils'; -import type { Json } from '@metamask/utils'; +import type { Hex, Json } from '@metamask/utils'; +import BigNumber from 'bignumber.js'; import { TransactionStatus } from '../types'; import type { @@ -183,3 +184,31 @@ export function padHexToEvenLength(hex: string) { return prefix + evenData; } + +/** + * Calculate the percentage difference between two hex values and determine if it is within a threshold. + * + * @param value1 - The first hex value. + * @param value2 - The second hex value. + * @param threshold - The percentage threshold for considering the values equal. + * @returns Whether the percentage difference is within the threshold. + */ +export function isPercentageDifferenceWithinThreshold( + value1: Hex, + value2: Hex, + threshold: number, +): boolean { + const bnValue1 = new BigNumber(value1); + const bnValue2 = new BigNumber(value2); + + if (bnValue1.isZero() && bnValue2.isZero()) { + return true; + } + + const difference = bnValue1.minus(bnValue2).abs(); + const average = bnValue1.plus(bnValue2).dividedBy(2); + + const percentageDifference = difference.dividedBy(average).multipliedBy(100); + + return percentageDifference.isLessThanOrEqualTo(threshold); +} diff --git a/yarn.lock b/yarn.lock index 06a9e70928..e160158ef0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3111,7 +3111,7 @@ __metadata: "@metamask/profile-sync-controller": "npm:^0.9.7" "@types/jest": "npm:^27.4.1" "@types/readable-stream": "npm:^2.3.0" - bignumber.js: "npm:^4.1.0" + bignumber.js: "npm:^9.1.2" contentful: "npm:^10.15.0" deepmerge: "npm:^4.2.2" firebase: "npm:^10.11.0" @@ -3673,6 +3673,7 @@ __metadata: "@types/jest": "npm:^27.4.1" "@types/node": "npm:^16.18.54" async-mutex: "npm:^0.5.0" + bignumber.js: "npm:^9.1.2" bn.js: "npm:^5.2.1" deepmerge: "npm:^4.2.2" eth-method-registry: "npm:^4.0.0" @@ -5318,14 +5319,7 @@ __metadata: languageName: node linkType: hard -"bignumber.js@npm:^4.1.0": - version: 4.1.0 - resolution: "bignumber.js@npm:4.1.0" - checksum: 10/b9a1761dd3c46ae2af44fb7c4c5587c57ba6815e6b4212c36f404984203866be01dd86a9882b2c65ebbed2625ed794c57a7955d78449ba1787843c3c8b556bc2 - languageName: node - linkType: hard - -"bignumber.js@npm:^9.0.1": +"bignumber.js@npm:^9.0.1, bignumber.js@npm:^9.1.2": version: 9.1.2 resolution: "bignumber.js@npm:9.1.2" checksum: 10/d89b8800a987225d2c00dcbf8a69dc08e92aa0880157c851c287b307d31ceb2fc2acb0c62c3e3a3d42b6c5fcae9b004035f13eb4386e56d529d7edac18d5c9d8