From 7b3149926a98257f7c3c99c2dbde9ed40b7fd869 Mon Sep 17 00:00:00 2001 From: Shane Terence Odlum Date: Tue, 3 Dec 2024 14:16:12 +0000 Subject: [PATCH] chore: validate that custodians did not change parameters when signing --- packages/site/CHANGELOG.md | 4 +- packages/snap/CHANGELOG.md | 4 +- packages/snap/snap.manifest.json | 3 +- .../src/features/error-message/render.tsx | 15 +++ packages/snap/src/lib/helpers/transaction.ts | 99 +++++++++++++++++-- packages/snap/src/requestsManager.ts | 33 +++++-- 6 files changed, 137 insertions(+), 21 deletions(-) create mode 100644 packages/snap/src/features/error-message/render.tsx diff --git a/packages/site/CHANGELOG.md b/packages/site/CHANGELOG.md index 444d4f0..0ce1555 100644 --- a/packages/site/CHANGELOG.md +++ b/packages/site/CHANGELOG.md @@ -13,5 +13,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial commit -[Unreleased]: https://github.com/MetaMask/snap-insitutional-wallet/compare/v0.0.1...HEAD -[0.0.1]: https://github.com/MetaMask/snap-insitutional-wallet/releases/tag/v0.0.1 +[Unreleased]: https://github.com/MetaMask/snap-institutional-wallet/compare/v0.0.1...HEAD +[0.0.1]: https://github.com/MetaMask/snap-institutional-wallet/releases/tag/v0.0.1 diff --git a/packages/snap/CHANGELOG.md b/packages/snap/CHANGELOG.md index 444d4f0..0ce1555 100644 --- a/packages/snap/CHANGELOG.md +++ b/packages/snap/CHANGELOG.md @@ -13,5 +13,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial commit -[Unreleased]: https://github.com/MetaMask/snap-insitutional-wallet/compare/v0.0.1...HEAD -[0.0.1]: https://github.com/MetaMask/snap-insitutional-wallet/releases/tag/v0.0.1 +[Unreleased]: https://github.com/MetaMask/snap-institutional-wallet/compare/v0.0.1...HEAD +[0.0.1]: https://github.com/MetaMask/snap-institutional-wallet/releases/tag/v0.0.1 diff --git a/packages/snap/snap.manifest.json b/packages/snap/snap.manifest.json index dc858dc..2d2b15d 100644 --- a/packages/snap/snap.manifest.json +++ b/packages/snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "git+https://github.com/MetaMask/snap-institutional-wallet.git" }, "source": { - "shasum": "pAM+McrqnMYKY0ctbnqvhb2POFwcXh5wdz8DnklnoXc=", + "shasum": "6KG5Omb+KFNCRvJ5uSSrIuAgJAGprnKFqj1PDPzsaN0=", "location": { "npm": { "filePath": "dist/bundle.js", @@ -35,6 +35,7 @@ "https://apps-portal.safe.global": {} }, "initialPermissions": { + "snap_notify": {}, "endowment:network-access": {}, "endowment:ethereum-provider": {}, "endowment:keyring": { diff --git a/packages/snap/src/features/error-message/render.tsx b/packages/snap/src/features/error-message/render.tsx new file mode 100644 index 0000000..1a87d04 --- /dev/null +++ b/packages/snap/src/features/error-message/render.tsx @@ -0,0 +1,15 @@ +/** + * Renders the onboarding interface. + * + * @param errorMessage - The error message to display. + * @returns The result of the dialog. + */ +export async function renderErrorMessage(errorMessage: string) { + await snap.request({ + method: 'snap_notify', + params: { + type: 'inApp', + message: errorMessage, + }, + }); +} diff --git a/packages/snap/src/lib/helpers/transaction.ts b/packages/snap/src/lib/helpers/transaction.ts index 1409cde..3f20ab1 100644 --- a/packages/snap/src/lib/helpers/transaction.ts +++ b/packages/snap/src/lib/helpers/transaction.ts @@ -1,9 +1,11 @@ +import type { TypedTransaction } from '@ethereumjs/tx'; import { TransactionFactory } from '@ethereumjs/tx'; import logger from '../../logger'; import { createCommon, formatTransactionData } from '../../util'; import { hexlify } from '../../util/hexlify'; import { TRANSACTION_TYPES } from '../constants'; +import type { EthSignTransactionRequest } from '../types/EthSignTransactionRequest'; import type { ITransactionDetails } from '../types/ITransactionDetails'; import type { IEIP1559TxParams, ILegacyTXParams } from '../types/ITXParams'; @@ -38,19 +40,10 @@ export class TransactionHelper { transaction: ITransactionDetails, chainId: string, ): Promise<{ v: string; r: string; s: string }> { - const common = createCommon(transaction, chainId); - if (transaction.signedRawTransaction) { logger.info('Transaction is signed', transaction.signedRawTransaction); - const signedRawTransaction = Buffer.from( - transaction.signedRawTransaction.substring(2), - 'hex', - ); - - const tx = TransactionFactory.fromSerializedData(signedRawTransaction, { - common, - }); + const tx = this.getTypedTransaction(transaction, chainId); return { v: hexlify(tx.v?.toString() ?? '0'), r: hexlify(tx.r?.toString() ?? '0'), @@ -78,4 +71,90 @@ export class TransactionHelper { s: tx.s, }; } + + static getTypedTransaction( + transactionDetails: ITransactionDetails, + chainId: string, + ): TypedTransaction { + if (!transactionDetails.signedRawTransaction) { + throw new Error('Transaction is not signed'); + } + + const common = createCommon(transactionDetails, chainId); + + const signedRawTransaction = Buffer.from( + transactionDetails.signedRawTransaction.substring(2), + 'hex', + ); + + const tx = TransactionFactory.fromSerializedData(signedRawTransaction, { + common, + }); + + return tx; + } + + /** + * We need to be certain that the custodian did not alter any of the fields + * which was previously allowed in MMI but cannot work in the institutional snap + * because the snap keyring does not allow us to return anything other than the signature + * + * @param request - The original request we got from the keyring + * @param transactionDetails - The transaction details we got from the custodian + */ + + static validateTransaction( + request: EthSignTransactionRequest, + transactionDetails: ITransactionDetails, + ): { + isValid: boolean; + error?: string; + } { + const tx = this.getTypedTransaction(transactionDetails, request.chainId); + // Validated nonce, gas price, maxFeePerGas, maxPriorityFeePerGas and gas limit + if (Number(request.nonce) !== Number(tx.nonce)) { + return { + isValid: false, + error: `Custodian altered the nonce from the request: ${Number( + request.nonce, + )} to ${Number(tx.nonce)}`, + }; + } + if ( + 'gasPrice' in request && + 'gasPrice' in tx && + Number(request.gasPrice) !== Number(tx.gasPrice) + ) { + return { + isValid: false, + error: `Custodian altered the gas price from the request: ${request.gasPrice} to ${tx.gasPrice}`, + }; + } + + if ( + 'maxFeePerGas' in request && + 'maxFeePerGas' in tx && + Number(request.maxFeePerGas) !== Number(tx.maxFeePerGas) + ) { + return { + isValid: false, + error: `Custodian altered the maxFeePerGas from the request: ${request.maxFeePerGas} to ${tx.maxFeePerGas}`, + }; + } + + if ( + 'maxPriorityFeePerGas' in request && + 'maxPriorityFeePerGas' in tx && + Number(request.maxPriorityFeePerGas) !== Number(tx.maxPriorityFeePerGas) + ) { + return { + isValid: false, + error: `Custodian altered the maxPriorityFeePerGas from the request: ${request.maxPriorityFeePerGas} to ${tx.maxPriorityFeePerGas}`, + }; + } + + return { + isValid: true, + }; + } } diff --git a/packages/snap/src/requestsManager.ts b/packages/snap/src/requestsManager.ts index 927e483..68e5e82 100644 --- a/packages/snap/src/requestsManager.ts +++ b/packages/snap/src/requestsManager.ts @@ -1,6 +1,7 @@ import { emitSnapKeyringEvent, KeyringEvent } from '@metamask/keyring-api'; import type { Json } from '@metamask/snaps-sdk'; +import { renderErrorMessage } from './features/error-message/render'; import { TransactionHelper } from './lib/helpers/transaction'; import type { ICustodianApi } from './lib/types'; import type { @@ -48,6 +49,15 @@ export class RequestManager { } async getChainIdFromPendingRequest(id: string): Promise { + const transactionRequest = this.getRequestParams(id); + if (!transactionRequest.chainId) { + throw new Error(`Request ${id} has no chainId`); + } + + return transactionRequest.chainId; + } + + getRequestParams(id: string): EthSignTransactionRequest { if (!this.#state.requests[id]) { throw new Error(`Request ${id} not found`); } @@ -59,12 +69,7 @@ export class RequestManager { throw new Error(`Request ${id} has invalid params`); } - const transactionRequest = requestParams[0] as EthSignTransactionRequest; - if (!transactionRequest.chainId) { - throw new Error(`Request ${id} has no chainId`); - } - - return transactionRequest.chainId; + return requestParams[0] as EthSignTransactionRequest; } async clearAllRequests(): Promise { @@ -145,6 +150,22 @@ export class RequestManager { chainId, ); + const validationResult = TransactionHelper.validateTransaction( + this.getRequestParams(requestId), + transactionResponse, + ); + + if (!validationResult.isValid) { + // First show a dialog with the error message + if (validationResult.error) { + const errorMessage = `Transaction ${custodianTransactionId} was signed by custodian but failed validation: ${validationResult.error}`; + await renderErrorMessage(errorMessage); + } + await this.emitRejectedEvent(requestId); + await this.removePendingRequest(requestId); + return; + } + const updatedTransaction = { ...request, fulfilled: true,