Skip to content

Commit

Permalink
fix: re-simulate transactions if security checks fail (#4792)
Browse files Browse the repository at this point in the history 
## Explanation

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->

This PR adds a mechanism to re-trigger of simulations if the security
provider mark transaction as `malicious` and the previous simulation
native balance change is different then the previous simulation.

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->

Fixes: MetaMask/MetaMask-planning#3380

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/transaction-controller`

- **ADDED**: Add mechanism to re-trigger of simulations if the security
provider mark transaction as `malicious` and the previous simulation
native balance change is different then the previous simulation.
- **ADDED**: Add `changeInSimulationData` property to `simulationData`
in order to detect change of simulation data.

## Checklist

- [X] I've updated the test suite for new or updated code as appropriate
- [X] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [X] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [X] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

---------

Co-authored-by: Matthew Walsh <[email protected]>
  • Loading branch information
@OGPoyraz @matthewwalsh0
OGPoyraz and matthewwalsh0 authored Oct 29, 2024
1 parent e692641 commit 1f0b94a
Show file tree
Hide file tree
Showing 10 changed files with 930 additions and 112 deletions.
8 changes: 4 additions & 4 deletions packages/transaction-controller/jest.config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 94.42,
functions: 97.45,
lines: 98.37,
statements: 98.38,
branches: 93.74,
functions: 97.51,
lines: 98.34,
statements: 98.35,
},
},

Expand Down
133 changes: 96 additions & 37 deletions packages/transaction-controller/src/TransactionController.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ import {
getTransactionLayer1GasFee,
updateTransactionLayer1GasFee,
} from './utils/layer1-gas-fee-flow';
import { shouldResimulate } from './utils/resimulate';
import { getSimulationData } from './utils/simulation';
import {
updatePostTransactionBalance,
Expand All @@ -112,9 +113,10 @@ jest.mock('./helpers/PendingTransactionTracker');
jest.mock('./utils/gas');
jest.mock('./utils/gas-fees');
jest.mock('./utils/gas-flow');
jest.mock('./utils/swaps');
jest.mock('./utils/layer1-gas-fee-flow');
jest.mock('./utils/resimulate');
jest.mock('./utils/simulation');
jest.mock('./utils/swaps');
jest.mock('uuid');

// TODO: Replace `any` with type
Expand Down Expand Up @@ -485,6 +487,7 @@ describe('TransactionController', () => {
getTransactionLayer1GasFee,
);
const getGasFeeFlowMock = jest.mocked(getGasFeeFlow);
const shouldResimulateMock = jest.mocked(shouldResimulate);

let mockEthQuery: EthQuery;
let getNonceLockSpy: jest.Mock;
Expand Down Expand Up @@ -1829,13 +1832,18 @@ describe('TransactionController', () => {
await flushPromises();

expect(getSimulationDataMock).toHaveBeenCalledTimes(1);
expect(getSimulationDataMock).toHaveBeenCalledWith({
chainId: MOCK_NETWORK.chainId,
data: undefined,
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
value: '0x0',
});
expect(getSimulationDataMock).toHaveBeenCalledWith(
{
chainId: MOCK_NETWORK.chainId,
data: undefined,
from: ACCOUNT_MOCK,
to: ACCOUNT_MOCK,
value: '0x0',
},
{
blockTime: undefined,
},
);

expect(controller.state.transactions[0].simulationData).toStrictEqual(
SIMULATION_DATA_MOCK,
Expand Down Expand Up @@ -5667,35 +5675,6 @@ describe('TransactionController', () => {
.toThrow(`TransactionsController: Can only call updateEditableParams on an unapproved transaction.
Current tx status: ${TransactionStatus.submitted}`);
});

it.each(['value', 'to', 'data'])(
'updates simulation data if %s changes',
async (param) => {
const { controller } = setupController({
options: {
state: {
transactions: [
{
...transactionMeta,
},
],
},
},
updateToInitialState: true,
});

expect(getSimulationDataMock).toHaveBeenCalledTimes(0);

await controller.updateEditableParams(transactionMeta.id, {
...transactionMeta.txParams,
[param]: ACCOUNT_2_MOCK,
});

await flushPromises();

expect(getSimulationDataMock).toHaveBeenCalledTimes(1);
},
);
});

describe('abortTransactionSigning', () => {
Expand Down Expand Up @@ -5826,4 +5805,84 @@ describe('TransactionController', () => {
);
});
});

describe('resimulate', () => {
it('triggers simulation if re-simulation detected on state update', async () => {
const { controller } = setupController({
options: {
state: {
transactions: [
{
...TRANSACTION_META_MOCK,
status: TransactionStatus.unapproved,
},
],
},
},
updateToInitialState: true,
});

expect(getSimulationDataMock).toHaveBeenCalledTimes(0);

shouldResimulateMock.mockReturnValueOnce({
blockTime: 123,
resimulate: true,
});

await controller.updateEditableParams(TRANSACTION_META_MOCK.id, {});

await flushPromises();

expect(getSimulationDataMock).toHaveBeenCalledTimes(1);
expect(getSimulationDataMock).toHaveBeenCalledWith(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_2_MOCK,
value: TRANSACTION_META_MOCK.txParams.value,
},
{
blockTime: 123,
},
);
});

it('does not trigger simulation loop', async () => {
const { controller } = setupController({
options: {
state: {
transactions: [
{
...TRANSACTION_META_MOCK,
status: TransactionStatus.unapproved,
},
],
},
},
updateToInitialState: true,
});

expect(getSimulationDataMock).toHaveBeenCalledTimes(0);

shouldResimulateMock.mockReturnValue({
blockTime: 123,
resimulate: true,
});

await controller.updateEditableParams(TRANSACTION_META_MOCK.id, {});

await flushPromises();

expect(getSimulationDataMock).toHaveBeenCalledTimes(1);
expect(getSimulationDataMock).toHaveBeenCalledWith(
{
from: ACCOUNT_MOCK,
to: ACCOUNT_2_MOCK,
value: TRANSACTION_META_MOCK.txParams.value,
},
{
blockTime: 123,
},
);
});
});
});
127 changes: 59 additions & 68 deletions packages/transaction-controller/src/TransactionController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ import { add0x } from '@metamask/utils';
import { Mutex } from 'async-mutex';
import { MethodRegistry } from 'eth-method-registry';
import { EventEmitter } from 'events';
import { cloneDeep, mapValues, merge, pickBy, sortBy, isEqual } from 'lodash';
import { cloneDeep, mapValues, merge, pickBy, sortBy } from 'lodash';
import { v1 as random } from 'uuid';

import { DefaultGasFeeFlow } from './gas-flows/DefaultGasFeeFlow';
Expand Down Expand Up @@ -105,6 +105,8 @@ import {
getAndFormatTransactionsForNonceTracker,
getNextNonce,
} from './utils/nonce';
import type { ResimulateResponse } from './utils/resimulate';
import { hasSimulationDataChanged, shouldResimulate } from './utils/resimulate';
import { getTransactionParamsWithIncreasedGasFee } from './utils/retry';
import { getSimulationData } from './utils/simulation';
import {
Expand Down Expand Up @@ -3546,15 +3548,17 @@ export class TransactionController extends BaseController<
note,
skipHistory,
skipValidation,
skipResimulateCheck,
}: {
transactionId: string;
note?: string;
skipHistory?: boolean;
skipValidation?: boolean;
skipResimulateCheck?: boolean;
},
callback: (transactionMeta: TransactionMeta) => TransactionMeta | void,
): Readonly<TransactionMeta> {
let updatedTransactionParams: (keyof TransactionParams)[] = [];
let resimulateResponse: ResimulateResponse | undefined;

this.update((state) => {
const index = state.transactions.findIndex(
Expand All @@ -3563,6 +3567,8 @@ export class TransactionController extends BaseController<

let transactionMeta = state.transactions[index];

const originalTransactionMeta = cloneDeep(transactionMeta);

// eslint-disable-next-line n/callback-return
transactionMeta = callback(transactionMeta) ?? transactionMeta;

Expand All @@ -3574,8 +3580,12 @@ export class TransactionController extends BaseController<
validateTxParams(transactionMeta.txParams);
}

updatedTransactionParams =
this.#checkIfTransactionParamsUpdated(transactionMeta);
if (!skipResimulateCheck && this.#isSimulationEnabled()) {
resimulateResponse = shouldResimulate(
originalTransactionMeta,
transactionMeta,
);
}

const shouldSkipHistory = this.isHistoryDisabled || skipHistory;

Expand All @@ -3592,64 +3602,35 @@ export class TransactionController extends BaseController<
transactionId,
) as TransactionMeta;

if (updatedTransactionParams.length > 0) {
this.#onTransactionParamsUpdated(
transactionMeta,
updatedTransactionParams,
);
}

return transactionMeta;
}

#checkIfTransactionParamsUpdated(newTransactionMeta: TransactionMeta) {
const { id: transactionId, txParams: newParams } = newTransactionMeta;

const originalParams = this.getTransaction(transactionId)?.txParams;

if (!originalParams || isEqual(originalParams, newParams)) {
return [];
}

const params = Object.keys(newParams) as (keyof TransactionParams)[];

const updatedProperties = params.filter(
(param) => newParams[param] !== originalParams[param],
);

log(
'Transaction parameters have been updated',
transactionId,
updatedProperties,
originalParams,
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);
if (resimulateResponse?.resimulate) {
this.#updateSimulationData(transactionMeta, {
blockTime: resimulateResponse.blockTime,
}).catch((error) => {
log('Error during re-simulation', error);
throw error;
});
}

return transactionMeta;
}

async #updateSimulationData(
transactionMeta: TransactionMeta,
{ traceContext }: { traceContext?: TraceContext } = {},
{
blockTime,
traceContext,
}: {
blockTime?: number;
traceContext?: TraceContext;
} = {},
) {
const { id: transactionId, chainId, txParams } = transactionMeta;
const {
id: transactionId,
chainId,
txParams,
simulationData: prevSimulationData,
} = transactionMeta;

const { from, to, value, data } = txParams;

let simulationData: SimulationData = {
Expand All @@ -3661,24 +3642,33 @@ export class TransactionController extends BaseController<
};

if (this.#isSimulationEnabled()) {
this.#updateTransactionInternal(
{ transactionId, skipHistory: true },
(txMeta) => {
txMeta.simulationData = undefined;
},
);

simulationData = await this.#trace(
{ name: 'Simulate', parentContext: traceContext },
() =>
getSimulationData({
chainId,
from: from as Hex,
to: to as Hex,
value: value as Hex,
data: data as Hex,
}),
getSimulationData(
{
chainId,
from: from as Hex,
to: to as Hex,
value: value as Hex,
data: data as Hex,
},
{
blockTime,
},
),
);

if (
blockTime &&
prevSimulationData &&
hasSimulationDataChanged(prevSimulationData, simulationData)
) {
simulationData = {
...simulationData,
isUpdatedAfterSecurityCheck: true,
};
}
}

const finalTransactionMeta = this.getTransaction(transactionId);
Expand All @@ -3698,6 +3688,7 @@ export class TransactionController extends BaseController<
{
transactionId,
note: 'TransactionController#updateSimulationData - Update simulation data',
skipResimulateCheck: Boolean(blockTime),
},
(txMeta) => {
txMeta.simulationData = simulationData;
Expand Down
Loading

0 comments on commit 1f0b94a

Please sign in to comment.