From 127ad0d6623e1c91390a404f843da203efd6e8bc Mon Sep 17 00:00:00 2001 From: carson <104383295+codebycarson@users.noreply.github.com> Date: Thu, 25 Jan 2024 14:04:46 -0700 Subject: [PATCH] Audit: MM Snap Edits (#117) * Sayfer MM Snap Edits - Throw an error when the account private key isn't found instead of silently failing - Removed `getWallet` function from snapWallet.ts as well as it's associated tests and mocks - Re arranged imports in api.ts - Added documentation for snap - Renamed jest.config.snap.js to jest.config.js so we don't have to explicitly pass in the config path to jest command * Adjusted snap manifest * MM Snap Coin Type Update - Revert change making coin type 60 instead of 118. MM snaps do not allow you to access coin type 118 as of now. * Added changeset * Reverted package.json version bump * Improved documentation --- .changeset/curvy-squids-cough.md | 5 ++ packages/metamask-snap/README.md | 66 ++++++++++++++++++- .../{jest.config.snap.js => jest.config.js} | 0 packages/metamask-snap/package.json | 2 +- packages/metamask-snap/snap.manifest.json | 4 +- packages/metamask-snap/src/api.ts | 20 +++--- packages/metamask-snap/src/snapWallet.ts | 12 ---- .../metamask-snap/src/tests/mocks/mocks.ts | 6 +- .../src/tests/mocks/output.mock.ts | 2 +- .../src/tests/snapWallet.spec.ts | 37 +---------- yarn.lock | 2 +- 11 files changed, 87 insertions(+), 69 deletions(-) create mode 100644 .changeset/curvy-squids-cough.md rename packages/metamask-snap/{jest.config.snap.js => jest.config.js} (100%) diff --git a/.changeset/curvy-squids-cough.md b/.changeset/curvy-squids-cough.md new file mode 100644 index 00000000..7d5c0cec --- /dev/null +++ b/.changeset/curvy-squids-cough.md @@ -0,0 +1,5 @@ +--- +'@sei-js/metamask-snap': patch +--- + +Fixes a silent return if the account private key is not found, adds documentation, removes unused getWallet function from @sei-js/metamask-snap all as fixes to code audit. diff --git a/packages/metamask-snap/README.md b/packages/metamask-snap/README.md index c44b505c..53cec0e7 100644 --- a/packages/metamask-snap/README.md +++ b/packages/metamask-snap/README.md @@ -1,3 +1,65 @@ -## Sei Metamask Snap +# Sei Metamask Snap -This snap is under active development and is not yet ready for use until this packages has been properly audited by a third party. +This documentation describes the functions and RPC request handler used in a MetaMask Snap to handle cryptocurrency transactions and account management using BIP44 standards. + +### `onRpcRequest` + +An RPC request handler that processes different request methods including `signDirect`, `signAmino`, and `getPrivateKey`. It handles the signing of transactions both in Amino and Direct mode, and also retrieves the private key. + +- **Syntax**: `onRpcRequest({ request }): Promise` +- **Parameters**: + - `request`: An object representing the RPC request. +- **Returns**: A Promise that resolves to a `TransactionSignature` or `PrivateKey`. + +## RPC Methods + +### `signDirect` + +Signs a transaction in Direct mode. + +- **Parameters**: + - `account_index`: The bip44 coin type 118 account index to use for signing. + - `signerAddress`: The address of the signer. + - `signDoc`: The document to be signed. +- **Error Handling**: Throws an error if the private key is not available or if the user denies the transaction. + +### `signAmino` + +Signs a transaction in Amino mode. + +- **Parameters**: + - `account_index`: The bip44 coin type 118 account index to use for signing. + - `signerAddress`: The address of the signer. + - `signDoc`: The document to be signed. + - `enableExtraEntropy`: Boolean flag for extra entropy. + - `isADR36`: Boolean flag for ADR-36 compliance. +- **Error Handling**: Throws an error if the private key is not available or if the user denies the transaction. + + +### `getPrivateKey` + +Retrieves the private key for a specified account index using MetaMask's `snap_getBip44Entropy` method and BIP44 standards and coin type 118. + +- **Syntax**: `getPrivateKey(account_index: number = 0): Promise` +- **Parameters**: + - `account_index` (number, optional): The index of the account, defaulting to 0. +- **Returns**: A Promise resolving to a `PrivateKey`. + + +## Local development and testing + +### Setup +Run `yarn` to install dependencies. + +### Running the snap locally +In order to run the snap locally simply run `yarn start` and the snap will be served at `localhost:8080`. + +### Testing the snap + +#### Test UI +This package has a helpful UI for testing all the exposed functions. + +`cd` into the `./test-ui` directory and run `yarn` to install dependencies. Then run `yarn dev` to start the test UI. The test UI will be served at `localhost:3000`. Please checkout the README.md in that directory to learn how to set the snap ID. + +#### Unit tests +Run `yarn test` to run the unit tests. diff --git a/packages/metamask-snap/jest.config.snap.js b/packages/metamask-snap/jest.config.js similarity index 100% rename from packages/metamask-snap/jest.config.snap.js rename to packages/metamask-snap/jest.config.js diff --git a/packages/metamask-snap/package.json b/packages/metamask-snap/package.json index 02528577..85cc6421 100644 --- a/packages/metamask-snap/package.json +++ b/packages/metamask-snap/package.json @@ -22,7 +22,7 @@ "serve": "mm-snap serve", "start": "mm-snap watch", "test": "yarn test:snap", - "test:snap": "jest --config ./jest.config.snap.js" + "test:snap": "jest" }, "dependencies": { "@metamask/snaps-types": "^3.1.0", diff --git a/packages/metamask-snap/snap.manifest.json b/packages/metamask-snap/snap.manifest.json index 08b9ec74..4354352e 100644 --- a/packages/metamask-snap/snap.manifest.json +++ b/packages/metamask-snap/snap.manifest.json @@ -1,5 +1,5 @@ { - "version": "1.0.0", + "version": "1.0.2", "description": "A Metamask snap for Sei built with TypeScript", "proposedName": "Sei", "repository": { @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/template-snap-monorepo.git" }, "source": { - "shasum": "BL8YslCsPGPRZSF95ze0fYvr9D/ybmtGXZn19/PqDLs=", + "shasum": "Lg7E4N7U729gA6Ikfk3KgiZpNOfDVOmwR01y7C4fZKg=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/metamask-snap/src/api.ts b/packages/metamask-snap/src/api.ts index 1c05b9a8..ff38c032 100644 --- a/packages/metamask-snap/src/api.ts +++ b/packages/metamask-snap/src/api.ts @@ -1,12 +1,12 @@ +import { BIP44CoinTypeNode, getBIP44AddressKeyDeriver } from '@metamask/key-tree'; import { OnRpcRequestHandler } from '@metamask/snaps-types'; -import { SignAminoRequest, SignDirectRequest } from './types'; +import { SignDoc } from 'cosmjs-types/cosmos/tx/v1beta1/tx'; import Long from 'long'; -import { sanitizedUint8Array } from './utils'; -import { getAminoPanel, getDirectPanel } from './ui'; -import { BIP44CoinTypeNode, getBIP44AddressKeyDeriver } from '@metamask/key-tree'; -import { SnapRequest } from './types'; import { SnapWallet } from './snapWallet'; -import { SignDoc } from 'cosmjs-types/cosmos/tx/v1beta1/tx'; + +import { SignAminoRequest, SignDirectRequest, SnapRequest } from './types'; +import { getAminoPanel, getDirectPanel } from './ui'; +import { sanitizedUint8Array } from './utils'; export const getPrivateKey = async (account_index: number = 0) => { const bip44CoinNode = (await snap.request({ @@ -27,7 +27,7 @@ export const onRpcRequest: OnRpcRequestHandler = async ({ request }) => { case 'signDirect': { const account = await getPrivateKey(account_index); - if (!account?.privateKey) return; + if (!account?.privateKey) throw new Error('Unable to get private key from MetaMask.'); const wallet = SnapWallet.create(account.privateKey); @@ -62,7 +62,7 @@ export const onRpcRequest: OnRpcRequestHandler = async ({ request }) => { const account = await getPrivateKey(account_index); - if (!account?.privateKey) return; + if (!account?.privateKey) throw new Error('Unable to get private key from MetaMask.'); const wallet = SnapWallet.create(account.privateKey); @@ -82,6 +82,6 @@ export const onRpcRequest: OnRpcRequestHandler = async ({ request }) => { return await getPrivateKey(account_index); } default: - throw new Error('Method not found.'); - } + throw new Error('Function not found.'); + } }; diff --git a/packages/metamask-snap/src/snapWallet.ts b/packages/metamask-snap/src/snapWallet.ts index fda8ef88..2dae331f 100644 --- a/packages/metamask-snap/src/snapWallet.ts +++ b/packages/metamask-snap/src/snapWallet.ts @@ -1,13 +1,10 @@ import { sign as signSecp256k1, getPublicKey as getSecp256k1PublicKey } from '@noble/secp256k1'; import { sha256 } from '@noble/hashes/sha256'; import { SignDoc } from 'cosmjs-types/cosmos/tx/v1beta1/tx'; -import { BIP44Node } from '@metamask/key-tree'; import { AccountData, encodeSecp256k1Signature, StdSignDoc } from '@cosmjs/amino'; import { Buffer } from 'buffer'; import { compressedPubKeyToAddress, serializeAminoSignDoc, serializeDirectSignDoc } from '@sei-js/core'; -import { sendReqToSnap } from './utils'; - export class SnapWallet { constructor(private privateKey: Uint8Array, private compressedPubKey: Uint8Array, private address: string) {} @@ -76,12 +73,3 @@ export class SnapWallet { }; } } - -export async function getWallet(account_index = 0, snapId: string): Promise { - const account: BIP44Node = await sendReqToSnap('getPrivateKey', { account_index }, snapId); - - if (account.privateKey) { - return SnapWallet.create(account.privateKey); - } - throw new Error(`Error creating sei wallet!`); -} diff --git a/packages/metamask-snap/src/tests/mocks/mocks.ts b/packages/metamask-snap/src/tests/mocks/mocks.ts index f30ffb7a..72e708f3 100644 --- a/packages/metamask-snap/src/tests/mocks/mocks.ts +++ b/packages/metamask-snap/src/tests/mocks/mocks.ts @@ -1,7 +1,3 @@ export const ACCOUNT_ONE_PRIVATE_KEY = '0x0ca350a5d50a08caf95d0922d3adf4117a1ea5b7c24b7c6021173c8d46eceb37'; -export const ACCOUNT_ONE_PUBLIC_KEY = - '0x0405794d2d33a8a8c7e21caa891a9cb5b4539fb4c2fb50e46b326b322474ce719763ca0a0c869a87640289ebe4a2c008822059bbf7564129bafe51676abc53b921'; export const ACCOUNT_ONE_ADDRESS = 'sei15u8zs9pqdjddgv8pkyyh6zvsg4ujs2y6s6cq6u'; -export const ACCOUNT_ONE_PUBKEY_BYTES = new Uint8Array([ - 3, 5, 121, 77, 45, 51, 168, 168, 199, 226, 28, 170, 137, 26, 156, 181, 180, 83, 159, 180, 194, 251, 80, 228, 107, 50, 107, 50, 36, 116, 206, 113, 151 -]); + diff --git a/packages/metamask-snap/src/tests/mocks/output.mock.ts b/packages/metamask-snap/src/tests/mocks/output.mock.ts index 7dd17077..d7132a54 100644 --- a/packages/metamask-snap/src/tests/mocks/output.mock.ts +++ b/packages/metamask-snap/src/tests/mocks/output.mock.ts @@ -303,7 +303,7 @@ const output = { } }, failure: { - invalidMethod: 'Method not found.' + invalidMethod: 'Function not found.' } }; diff --git a/packages/metamask-snap/src/tests/snapWallet.spec.ts b/packages/metamask-snap/src/tests/snapWallet.spec.ts index cc5d36a4..9be98adb 100644 --- a/packages/metamask-snap/src/tests/snapWallet.spec.ts +++ b/packages/metamask-snap/src/tests/snapWallet.spec.ts @@ -1,6 +1,5 @@ -import { getWallet, SnapWallet } from '../snapWallet'; -import * as Utils from '../utils'; -import { ACCOUNT_ONE_ADDRESS, ACCOUNT_ONE_PRIVATE_KEY, ACCOUNT_ONE_PUBKEY_BYTES, ACCOUNT_ONE_PUBLIC_KEY } from './mocks/mocks'; +import { SnapWallet } from '../snapWallet'; +import { ACCOUNT_ONE_ADDRESS, ACCOUNT_ONE_PRIVATE_KEY } from './mocks/mocks'; describe('SnapWallet', () => { it('should create a SnapWallet instance', () => { @@ -17,35 +16,3 @@ describe('SnapWallet', () => { expect(accounts[0].algo).toBe('secp256k1'); }); }); - -describe('getWallet', () => { - it('should create a SnapWallet instance via getWallet', async () => { - const expectedBip44Node = { - depth: 5, - parentFingerprint: 567825211, - index: 0, - privateKey: ACCOUNT_ONE_PRIVATE_KEY, - publicKey: ACCOUNT_ONE_PUBLIC_KEY, - chainCode: '0xf2a762ae70bddda87be9629e0be5857be287724fe6c5cc69c4c8cb1dcdccd089' - }; - - const mock = jest.spyOn(Utils, 'sendReqToSnap'); - mock.mockResolvedValue(expectedBip44Node); - - const wallet = await getWallet(0, 'npm:@sei-js/metamask-snap'); - expect(wallet).toBeInstanceOf(SnapWallet); - - const expectedAccounts = [ - { - address: ACCOUNT_ONE_ADDRESS, - algo: 'secp256k1', - pubkey: ACCOUNT_ONE_PUBKEY_BYTES - } - ]; - - const accountsResponse = wallet.getAccounts(); - expect(accountsResponse).toStrictEqual(expectedAccounts); - expect(wallet.signAmino).toBeDefined(); - expect(wallet.signDirect).toBeDefined(); - }); -}); diff --git a/yarn.lock b/yarn.lock index f86077ad..79423558 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6405,7 +6405,7 @@ babel-jest@28.1.3: babel-jest@^29.7.0: version "29.7.0" - resolved "https://registry.npmjs.org/babel-jest/-/babel-jest-29.7.0.tgz#f4369919225b684c56085998ac63dbd05be020d5" + resolved "https://registry.yarnpkg.com/babel-jest/-/babel-jest-29.7.0.tgz#f4369919225b684c56085998ac63dbd05be020d5" integrity sha512-BrvGY3xZSwEcCzKvKsCi2GgHqDqsYkOP4/by5xCgIwGXQxIEh+8ew3gmrE1y7XRR6LHZIj6yLYnUi/mm2KXKBg== dependencies: "@jest/transform" "^29.7.0"