Wallet Guard

Description

The snap prompts users to input the wallet address to be monitored. Users can set wallet addreses that do not adhere to the common Ethereum address format. The user input is not sanitized. This could lead to various injection vulnerabilities such as markdown or control character injections that could break other components. In particular, the address is sent to the API as a URL query parameter. A malicious attacker could try using that to mount URL injection attacks.

packages/snap/src/index.ts:L50-L61

if (
  request.method === RpcRequestMethods.UpdateAccount &&
  'walletAddress' in request.params &&
  typeof request.params.walletAddress === 'string'
) {
  const { walletAddress } = request.params;

  if (!walletAddress) {
    throw new Error('no wallet address provided');
  }

  updateWalletAddress(walletAddress);

Recommendation

Sanitize the address string input by the user and reject all addresses that do not adhere to the Ethereum address format.

Description

The snap code sends a request to the Wallet Guard API with a random UUID crypto.randomUUID() generated by the client. We would like to underline that the API should never trust clients’ randomness nor assume any property about it. Relying on client-generated randomness for the API could lead to many vulnerabilities, such as replay attacks or collision issues due to the inability to ensure uniqueness. The varying algorithms used by clients may be subpar or even compromised. As this id is not used anywhere else in the snap code, we assume that it might be used on the API side. Because the API is not in scope for this review, we don’t have access to the code and cannot tell whether this pseudo-random UUID is used in a safe way.

packages/snap/src/http/fetchTransaction.ts:L32-L40

const simulateRequest: SimulateRequestParams = {
  id: crypto.randomUUID(),
  chainID: mappedChainId,
  signer: transaction.from as string,
  origin: transactionOrigin as string,
  method: transaction.method as string,
  transaction,
  source: 'SNAP',
};

Recommendation

Don’t rely on clients’ randomness on the API. Instead, the server should assign a unique ID to every incoming request.

Description

The Metamask Snaps API does not guarantee that the properties from and method of the transaction object are defined. Depending on the transaction type, it could happen that these properties are not defined. This would result in a runtime error when undefined is casted to string.

packages/snap/src/http/fetchTransaction.ts:L32-L40

const simulateRequest: SimulateRequestParams = {
  id: crypto.randomUUID(),
  chainID: mappedChainId,
  signer: transaction.from as string,
  origin: transactionOrigin as string,
  method: transaction.method as string,
  transaction,
  source: 'SNAP',
};

Recommendation

One should check whether properties from, and method are defined, before explicitly casting them to a string. This could be done by introducing a hasProperty utility function for instance.

Description

The toFixed(2) method rounds the transaction value string to 2 decimals. For transactions with fiatValue < 0.005, the function returns 0, meaning the component will display a transaction with zero value to the user, even if the transaction has a small yet non-zero value. This is not a good idea as it might trick the user. In that case, it would be better to default to the smallest value that can represented (i.e. 0.01) instead of 0.

packages/snap/src/components/stateChanges/AssetChangeComponent.ts:L18

const fiatValue = Number(stateChange.fiatValue).toFixed(2);

Recommendation

If fiatValue < 0.005, consider displaying a value of 0.01 to the user, instead of 0.

Description

The code is missing NatSpec documentation in many places. NatSpec documentation plays an important role in improving code comprehension and maintenance. Adding NatSpec documentation to functions with significant logic that provides clear explanations of behavior, inputs, and outputs enhances code readability, transparency, and maintainability of the codebase.

Recommendation

We recommend adding NatSpec documentation to every function that contains significant logic. Especially all the Snaps handlers. This will improve the readability, transparency, and maintainability of the codebase. We also recommend adding a detailed high-level documentation about the Snaps features, components, and permissions in the README.

Description

The function formatFiatValue formats a number to a string that is displayed to the user. The function formats numbers with at most 2 decimal digits, removes the trailing zeros, and adds commas as thousands separators.

The function first converts the number to a string representing the number in fixed-point notation. Then, it uses regex to remove the trailing zeros if they exist. Finally, it adds the thousands separators.

packages/snap/src/utils/helpers.ts:L16-L26

export const formatFiatValue = (
  fiatValue: string,
  maxDecimals: number,
): string => {
  const fiatWithRoundedDecimals = Number(fiatValue)
    .toFixed(maxDecimals) // round to maxDecimals
    .replace(/\.00$/u, ''); // removes 00 if it exists

  const fiatWithCommas = numberWithCommas(fiatWithRoundedDecimals); // add commas
  return `$${fiatWithCommas}`;
};

The design of the function is unnecessarily complex. The whole design could be simplified using the native toLocaleString() function with appropriate parameters.

Recommendation

Simplify the design by using the native toLocaleString function. For instance, the function could be used as follows toLocaleString('en-US',{minimumFractionDigits: 0, maximumFractionDigits: 2})

Description

Currently, there is no easy way to disable wallet approval monitoring and/or transaction simulation apart from uninstalling the snap. Users might want to opt out of wallet monitoring or disable transaction simulation selectively e.g., for privacy concerns.

Recommendation

We would recommend implementing a mechanism that allows users to selectively disable the snap features.

Description

The following dependencies are only used for development purpose and should therefore be listed as “devDependencies” instead of “dependencies” in the package.json file. Indeed, the TypeScript code is compiled into a bundle, which is released. Meaning the snap “production” code should not contain any external dependency.

packages/snap/package.json:L28-L31

"dependencies": {
  "@metamask/snaps-types": "^0.32.2",
  "@metamask/snaps-ui": "^0.32.2"
},

Recommendation

List the dependencies as “devDependencies”.

Description

The package.json file is missing the author name, the link to the project homepage, and to the bug tracker.

Recommendation

According to package publishing best practices, we recommend adding those elements to the package.json file.

Description

The onRpcRequest() handler returns early if walletAddress is not defined.

packages/snap/src/index.ts:L57-L59

if (!walletAddress) {
  throw new Error('no wallet address provided');
}

Thus, the extra ‘if’ check before calling snap.request() is superfluous and can be removed.

packages/snap/src/index.ts:L57-L64

if (!walletAddress) {
  throw new Error('no wallet address provided');
}

updateWalletAddress(walletAddress);

if (walletAddress) {
  await snap.request({

Recommendation

Remove the extra ‘if’ check.

Description

The NatSpec comment indicates that onRpcRequest() returns “the result of snap_dialog” while the method either does not return anything, or returns the Ethereum address of the monitored wallet.

packages/snap/src/index.ts:L24-L34

/**
 * Handle incoming JSON-RPC requests, sent through `wallet_invokeSnap`.
 *
 * @param args - The request handler args as object.
 * @param args.origin - The origin of the request, e.g., the website that
 * invoked the snap.
 * @param args.request - A validated JSON-RPC request object.
 * @returns The result of `snap_dialog`.
 * @throws If the request method is not valid for this snap.
 */

Recommendation

Fix the comment.

Description

The snap allows the user to set an arbitrary wallet address to be monitored for dangerous approvals. This feature is only of limited use and could be improved by:

• Allowing to specify multiple addresses to monitor (a wallet typically consists of many accounts that are managed under the wallet key)
• Allowing users to fetch connected addresses via the ethereum API directly instead of requiring the user to input valid accounts
• For privacy reasons, allowing users to opt out of transaction analytics on a per-account basis (Currently, every transaction and transaction origin is sent to the API, even if no monitored wallet address is set).

Description

Consider adding the snap package version to the API requests in order to get insights about what snap versions are used in the field. This could be useful for future debugging and forensics when multiple snap versions will coexist.

packages/snap/src/http/fetchTransaction.ts:L32-L40

const simulateRequest: SimulateRequestParams = {
  id: crypto.randomUUID(),
  chainID: mappedChainId,
  signer: transaction.from as string,
  origin: transactionOrigin as string,
  method: transaction.method as string,
  transaction,
  source: 'SNAP',
};

packages/snap/src/types/simulateApi.ts:L25-L35

export type SimulateRequestParams = {
  id: string;
  chainID: string;
  signer: string;
  origin: string;
  method: string;
  transaction: {
    [key: string]: Json;
  };
  source: 'SNAP';
};