chore: final pre-audit fixes (#37)

* chore: final pre-audit fixes

* chore: update shasum

* chore: fix test

* chore: fix linter

Author: shane-t

packages/api/src/index.ts

@@ -13,13 +13,13 @@ import type http from 'http';
 import custodianRequests from './custodian/requests';
 import { methodMapping } from './method-mapping';
 import doc from './openrpc.json';
+import clearRequestsHandler from './routes/clearRequestsHandler';
 import listRequestsHandler from './routes/listRequestsHandler';
 import tokenHandler from './routes/token-handler';
 import type { UpdateSignedMessageRequest } from './routes/updateSignedMessageHandler';
 import updateSignedMessageHandler from './routes/updateSignedMessageHandler';
 import type { UpdateTransactionRequest } from './routes/updateTransactionHandler';
 import updateTransactionHandler from './routes/updateTransactionHandler';
-import clearRequestsHandler from './routes/clearRequestsHandler';
 
 dotenv.config();
 

packages/snap/snap.manifest.json

@@ -7,7 +7,7 @@
     "url": "git+https://github.com/MetaMask/snap-institutional-wallet.git"
   },
   "source": {
-    "shasum": "xC69NMxzQ9d+X0HrsOyJJM6Pna8zpxzeSdt3WVYySaw=",
+    "shasum": "fxx/F8vsR2zv5EvcSLSyb2tXsCvDAYQx/2vclozmAzA=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

packages/snap/src/features/homepage/components/CustodianList.tsx

@@ -70,7 +70,7 @@ export const CustodianList: SnapComponent<CustodianListProps> = ({
           solution
         </Text>
         {custodianMetadata
-          .filter((custodian) => custodian.enabled)
+          .filter((custodian) => custodian.production)
           .map((custodian) => (
             <Section
               key={custodian.name}

packages/snap/src/index.ts

@@ -1,14 +1,13 @@
+/* eslint-disable @typescript-eslint/no-throw-literal */
 import type { JsonRpcRequest } from '@metamask/keyring-api';
-import {
-  MethodNotSupportedError,
-  handleKeyringRequest,
-} from '@metamask/keyring-api';
+import { handleKeyringRequest } from '@metamask/keyring-api';
 import {
   type UserInputEvent,
   type OnCronjobHandler,
   type OnUserInputHandler,
   type OnHomePageHandler,
   UnauthorizedError,
+  MethodNotFoundError,
 } from '@metamask/snaps-sdk';
 import type {
   Json,
@@ -34,6 +33,7 @@ import type { SnapContext } from './lib/types/Context';
 import { CustodianApiMap, CustodianType } from './lib/types/CustodianType';
 import logger from './logger';
 import { InternalMethod, originPermissions } from './permissions';
+// @audit - this file needs unittests
 
 /**
  * Verify if the caller can call the requested method.
@@ -155,17 +155,17 @@ export const onRpcRequest: OnRpcRequestHandler = async ({
         const requestManager = await getRequestManager();
         return await requestManager.clearAllRequests();
       }
-      throw new MethodNotSupportedError(request.method);
+      throw new MethodNotFoundError(request.method);
     }
 
     default: {
-      throw new MethodNotSupportedError(request.method); // @audit-info or MethodNotFoundError 👉 https://docs.metamask.io/snaps/how-to/communicate-errors/#import-and-throw-errors
+      throw new MethodNotFoundError(request.method);
     }
   }
 };
 
 export const onKeyringRequest: OnKeyringRequestHandler = async ({
-  origin, // @audit specify ts types
+  origin,
   request,
 }: {
   origin: string;
@@ -176,16 +176,17 @@ export const onKeyringRequest: OnKeyringRequestHandler = async ({
     JSON.stringify(request, undefined, 2),
   );
 
+  // assert(request.params, KeyringRequestStruct);
+
   // Check if origin is allowed to call method.
   if (!hasPermission(origin, request.method)) {
-    // @audit - enforce runtime type/input validation! user superstruct from metamask
     throw new Error(
       `Origin '${origin}' is not allowed to call '${request.method}'`,
     );
   }
 
-  // Handle keyring methods.
-  return handleKeyringRequest(await getKeyring(), request); // @audit - handleKeyringRequest is async, might req. await? (else returns promise of promise? dblcheck with MM team)
+  const keyring = await getKeyring();
+  return handleKeyringRequest(keyring, request);
 };
 
 // Improved polling function

packages/snap/src/keyring.test.ts

@@ -9,14 +9,14 @@ import type { ICustodianApi } from './lib/types/ICustodianApi';
 jest.mock('./lib/custodian-types/custodianMetadata', () => ({
   custodianMetadata: {
     BitGo: {
-      enabled: false,
+      production: false,
       apiBaseUrl: 'https://mock-url.com',
       apiVersion: 'BitGo',
       custodianPublishesTransaction: true,
       iconUrl: 'https://mock-url.com/icon.svg',
     },
     ECA3: {
-      enabled: true,
+      production: true,
       apiBaseUrl: 'https://mock-url.com',
       apiVersion: 'ECA3',
       custodianPublishesTransaction: false,

packages/snap/src/keyring.tsx

@@ -299,14 +299,18 @@ export class CustodialKeyring implements Keyring {
 
     let deepLink: CustodianDeepLink | null = null;
 
+    // Custodians may not support all methods
+    // but we don't store this anywhere, because they can implement them later
+    // We rely on the custodian api to throw an error if the deeplink method is not supported
+    // and generate a default message if that is the case
+
     try {
       if (request.request.method === EthMethod.SignTransaction) {
         const custodianApi = await this.getCustodianApiForAddress(address);
         deepLink = (await custodianApi.getTransactionLink(
           custodianId,
         )) as CustodianDeepLink;
       } else {
-        // @audit - explicitly check for methods in account config; throw methodNotFound if not exists
         const custodianApi = await this.getCustodianApiForAddress(address);
         deepLink = (await custodianApi.getSignedMessageLink(
           custodianId,
@@ -333,9 +337,7 @@ export class CustodialKeyring implements Keyring {
     params: Json,
     keyringRequest: KeyringRequest,
   ): Promise<string> {
-    switch (
-      method // @audit input validation
-    ) {
+    switch (method) {
       case EthMethod.PersonalSign: {
         const [message, from] = params as [string, string];
         const custodianApi = await this.getCustodianApiForAddress(from);

packages/snap/src/lib/custodian-types/custodianMetadata.ts

@@ -6,7 +6,7 @@ export type CustodianMetadata = {
   name: string;
   legacyName?: string;
   displayName: string | null;
-  enabled: boolean | null;
+  production: boolean | null;
   apiVersion: CustodianType;
   custodianPublishesTransaction: boolean;
   iconUrl: string | null;
@@ -47,7 +47,7 @@ export const custodianMetadata: (
     refreshTokenUrl: null,
     name: 'bitgo-test',
     displayName: 'BitGo Test',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://app.bitgo-test.com/defi/v2',
     apiVersion: CustodianType.BitGo,
     custodianPublishesTransaction: true,
@@ -62,7 +62,7 @@ export const custodianMetadata: (
     name: 'bitgo-prod',
     legacyName: 'bitgo',
     displayName: 'BitGo',
-    enabled: true,
+    production: true,
     apiBaseUrl: 'https://app.bitgo.com/defi/v2',
     apiVersion: CustodianType.BitGo,
     custodianPublishesTransaction: true,
@@ -76,7 +76,7 @@ export const custodianMetadata: (
     refreshTokenUrl: null,
     name: 'cactus',
     displayName: 'Cactus Custody',
-    enabled: true,
+    production: true,
     apiVersion: CustodianType.Cactus,
     custodianPublishesTransaction: true,
     iconUrl:
@@ -91,7 +91,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'http://localhost:8090/oauth/token',
     name: 'gk8-prod',
     displayName: 'GK8 ECA-1',
-    enabled: true,
+    production: true,
     apiBaseUrl: 'http://localhost:8090',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -104,7 +104,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'http://localhost:8090/oauth/token',
     name: 'gk8-eca3-prod',
     displayName: 'GK8 ECA-3',
-    enabled: true,
+    production: true,
     apiBaseUrl: 'http://localhost:8090',
     apiVersion: CustodianType.ECA3,
     custodianPublishesTransaction: true,
@@ -118,7 +118,7 @@ export const custodianMetadata: (
       'https://safe-mmi.staging.gnosisdev.com/api/v1/oauth/token/',
     name: 'gnosis-safe-dev',
     displayName: 'Safe',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://safe-mmi.staging.gnosisdev.com/api',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -132,7 +132,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://safe-mmi.safe.global/api/v1/oauth/token/',
     name: 'safe-prod',
     displayName: 'Safe',
-    enabled: true,
+    production: true,
     apiBaseUrl: 'https://safe-mmi.safe.global/api',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -146,7 +146,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://safe-mmi.staging.5afe.dev/api/v1/oauth/token/',
     name: 'gnosis-safe-staging',
     displayName: 'Gnosis Safe Staging',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://safe-mmi.staging.5afe.dev/api',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -161,7 +161,7 @@ export const custodianMetadata: (
       'https://saturn-custody.metamask-institutional.io/oauth/token',
     name: 'saturn-prod',
     displayName: 'Saturn Custody',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://saturn-custody.metamask-institutional.io/eth',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -174,7 +174,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://api.mpcvault.com/mmi/token-refresh',
     name: 'mpcvault-prod',
     displayName: 'MPCVault',
-    enabled: true,
+    production: true,
     apiBaseUrl: 'https://api.mpcvault.com/mmi',
     apiVersion: CustodianType.ECA3,
     custodianPublishesTransaction: true,
@@ -189,7 +189,7 @@ export const custodianMetadata: (
       'https://neptune-custody.metamask-institutional.io/oauth/token',
     name: 'neptune-custody-prod',
     displayName: 'Neptune Custody',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://neptune-custody.metamask-institutional.io/eth',
     apiVersion: CustodianType.ECA3,
     custodianPublishesTransaction: false,
@@ -203,7 +203,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://api-preprod.uat.zodia.io/oauth/token',
     name: 'zodia-preprod',
     displayName: 'Zodia Preprod',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://api-preprod.uat.zodia.io',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -216,7 +216,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://mmi.fireblocks.io/v1/auth/access',
     name: 'fireblocks-prod',
     displayName: 'Fireblocks',
-    enabled: true,
+    production: true,
     apiBaseUrl: 'https://mmi.fireblocks.io',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -230,7 +230,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://zapi.custody.zodia.io/oauth/token',
     name: 'zodia-prod',
     displayName: 'Zodia',
-    enabled: true,
+    production: true,
     apiBaseUrl: 'https://zapi.custody.zodia.io',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -243,7 +243,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://api.sit.zodia.io/oauth/token',
     name: 'zodia-sit',
     displayName: 'Zodia SIT',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://api.sit.zodia.io',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -256,7 +256,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://api-qa.qa.zodia.io/oauth/token',
     name: 'zodia-qa',
     displayName: 'Zodia QA',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://api-qa.qa.zodia.io',
     apiVersion: CustodianType.ECA1,
     custodianPublishesTransaction: true,
@@ -269,7 +269,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'http://localhost:8090/oauth/token',
     name: 'gk8-eca3-dev',
     displayName: 'GK8',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'http://localhost:8090',
     apiVersion: CustodianType.ECA3,
     custodianPublishesTransaction: true,
@@ -282,7 +282,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://api.dev.mpcvault.com/mmi/token-refresh',
     name: 'mpcvault-dev',
     displayName: 'MPCVault',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://api.dev.mpcvault.com/mmi',
     apiVersion: CustodianType.ECA3,
     custodianPublishesTransaction: false,
@@ -296,7 +296,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://gamma.signer.cubist.dev/v0/oauth/token',
     name: 'cubist-gamma',
     displayName: 'Cubist Gamma',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://gamma.signer.cubist.dev/v0/mmi',
     apiVersion: CustodianType.ECA3,
     custodianPublishesTransaction: false,
@@ -309,7 +309,7 @@ export const custodianMetadata: (
     refreshTokenUrl: 'https://dg5z0qnzb9s65.cloudfront.net/v0/oauth/token',
     name: 'cubist-test',
     displayName: 'Cubist Test',
-    enabled: false,
+    production: false,
     apiBaseUrl: 'https://dg5z0qnzb9s65.cloudfront.net/v0/mmi',
     apiVersion: CustodianType.ECA3,
     custodianPublishesTransaction: false,
@@ -325,7 +325,7 @@ export const custodianMetadata: (
     custodianPublishesTransaction: false,
     name: 'local-dev',
     displayName: 'Local Dev',
-    enabled: false,
+    production: false,
     iconUrl:
       'https://dev.metamask-institutional.io/custodian-icons/neptune-icon.svg',
     isManualTokenInputSupported: true,

packages/snap/src/permissions.test.ts

@@ -16,6 +16,7 @@ describe('Permissions', () => {
       jest.mock('./lib/custodian-types/custodianMetadata', () => ({
         custodianMetadata: [
           {
+            production: true,
             allowedOnboardingDomains: ['example.com', 'test.com'],
           },
         ],

packages/snap/src/permissions.ts

@@ -27,9 +27,14 @@ originPermissions.set(metamask, metamaskPermissions);
 custodianMetadata.forEach((custodian) => {
   if (custodian.allowedOnboardingDomains) {
     // exclude localhost
+
+    if (!config.dev && !custodian.production) {
+      return;
+    }
+
     custodian.allowedOnboardingDomains.forEach((domain) => {
-      // @audit - includes dev endpoints that should be excluded in prod
-      originPermissions.set(domain, new Set([InternalMethod.Onboard])); // @audit - should enforce HTTPS (this is a trust module; no more insecure transports); check if https prefix, else add it
+      // Due to a quirk of the snap SDK, we need to allow the onboarding domain as a bare domain
+      originPermissions.set(domain, new Set([InternalMethod.Onboard]));
       originPermissions.set(
         `https://${domain}`,
         new Set([InternalMethod.Onboard]),
@@ -51,9 +56,9 @@ const localhostPermissions = new Set([
   KeyringRpcMethod.GetRequest,
   // Custom methods
   InternalMethod.Onboard,
-  InternalMethod.ClearAllRequests, // @audit-ok only local dev
+  InternalMethod.ClearAllRequests,
 ]);
 
 if (config.dev) {
-  originPermissions.set('http://localhost:8000', localhostPermissions); // @audit-ok - remove for prod
+  originPermissions.set('http://localhost:8000', localhostPermissions);
 }