chore: validate that custodians did not change parameters when signing

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

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

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": {

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/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,