chore: validate that custodians did not change parameters when signing

Author: shane-t

packages/snap/snap.manifest.json

@@ -7,7 +7,7 @@
     "url": "git+https://github.com/MetaMask/snap-institutional-wallet.git"
   },
   "source": {
-    "shasum": "pAM+McrqnMYKY0ctbnqvhb2POFwcXh5wdz8DnklnoXc=",
+    "shasum": "A/ave6b+Ko6b+/uCp/2aBbK7++A/IfWIKQUni8CFfGI=",
     "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": {

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,
+    },
+  });
+}

packages/snap/src/lib/helpers/interface.ts

@@ -1,6 +1,7 @@
 import type {
   ComponentOrElement,
   DialogResult,
+  DialogType,
   GetInterfaceStateResult,
   Json,
   ResolveInterfaceResult,
@@ -91,13 +92,18 @@ export async function resolveInterface(
  * Shows a dialog using the provided ID.
  *
  * @param id - The ID for the dialog.
+ * @param type - The type of dialog to display.
  * @returns A promise that resolves to a string.
  */
-export async function showDialog(id: string): Promise<DialogResult> {
+export async function showDialog(
+  id: string,
+  type?: DialogType,
+): Promise<DialogResult> {
   return snap.request({
     method: 'snap_dialog',
     params: {
       id,
+      ...(type ? { type } : {}),
     },
   });
 }

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,
+    };
+  }
 }

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<string> {
+    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<void> {
@@ -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,