MetaMask/Partner Snaps - Shapeshift Snap

Description

When a normal dapp requests MetaMask to sign an arbitrary message, the message is displayed to the user for confirmation before signing it. Requiring explicit user consent and displaying the message to be signed is crucial to ensure that the user has full control over what messages are signed on their behalf.

The shapeshift snap exposes an RPC endpoint for ethereum (and an undocumented one for AVAX) that allows bypassing user consent. When invoking the shapeshift snap with the eth_signMessage RPC method, the snap signs the message right away, silently, without requiring consent from the wallet owner or notifying them of the fact that the dapp is signing with a HD wallet account on their behalf. This severely undermines security restrictions by the MetaMask that ensure that the end-user has full control over what is being signed, giving them the option to reject signing.

For comparison, eth_signTransaction ask for user confirmation while eth_signMessage does not a.

The relevant code can be found here:

packages/snap/src/rpc/evm/common/EVMSigner.ts:L37-L47

async signMessage({ message }: SignMessageParamsType<T>): Promise<SignMessageResponseType<T>> {
  try {
    return (await this.signer.ethSignMessage(
      message as SignerSignMessageType<T>,
    )) as SignMessageResponseType<T>
  } catch (error) {
    this.logger.error(message, { fn: 'ethSignMessage' }, error)
    return Promise.reject(error)
  }
}

It also affects the avax implementation:

packages/snap/src/rpc/evm/avalanche/handlers.ts:L34-L45

export const avalancheSignMessage = async (
  params: AvalancheSignMessageParams,
): Promise<AvalancheSignMessageResponse> => {
  try {
    const avalancheSigner = new AvalancheSigner()
    await avalancheSigner.initialize()
    return await avalancheSigner.signMessage(params)
  } catch (error) {
    moduleLogger.error({ fn: 'avalancheSignMessage' }, error)
    return Promise.reject(error)
  }
}

Examples

A dapp can invoke eth_signMessage which will not require user confirmation and silently return a message that is signed with any of the users HD wallet accounts.

await window.ethereum.request({
    method: 'wallet_invokeSnap',
    params: {
      snapId: "local:http://localhost:9000",
      request: { method: 'eth_signMessage', params: {message: {addressNList: [0x80000000 + 44, 0x80000000 + 60, 0x80000000 + 0, 0, 1], message:"hi"}, snapId: "local:http://localhost:9000" }},
    }});
{address: '0xBaB66CfA59757200c90c79BC6e2aEe4bFBe382Be', signature: '0x5c04bcc1ca73e9f9d4bf3642150407c01c189d784dd90349…e03ca8ec026ec6062b3d708c5fedbca0f2427282620be8c1b'}

Recommendation

Follow exactly the same flow MetaMask already implemented when signing arbitrary messages. Log signing requests, surface them to the user, ask for confirmation, and reject them by default (timeout). Display the origin on signing request dialogues and present the data to be signed and the account in a human-readable, understandable way while showing the original data to be signed, too.

Description

As the Snap Outline in this report mentions, the ShapeShift snap requests access to the BIP32 entropy for the Ethereum private keys. This effectively allows the ShapeShift snap to manage MetaMasks Ethereum keys directly, which comes with great responsibility. To avoid undermining established security controls put in place by the MetaMask team, the snap would have to replicate the same security functionality not to degrade the security posture of MetaMask altogether.

For reference, please take a look at issue 4.4, issue 4.1 , issue 4.9 .

Recommendation

We recommend using the Metamask provider exposed via the endowment:ethereum-provider RPC endpoint to perform Ethereum operations instead of managing the Ethereum keys and low-level operations directly. This avoids bypassing MetaMask security controls but falling back to proven and battle-tested user confirmation dialogs instead.

Moreover, we also asked the MM team to provide a more robust account management API that is not based on giving full low-level account access to Snaps. This would enable Snaps to perform signing operations with control over cryptographic parameters (e.g., BIP-44 derivation path) without accessing the root entropy. This will significantly decrease the risks for the end-user.

Description

The snap requests permission endowment:network-access to interact with external entities over HTTP/fetch. While the sandbox/demo dapp may use the fetch API in its context as a web app, the requested permission is only relevant for the snap and the snap never calls the fetch() API. Hence, the permission is requested but never used.

Requesting more permissions than necessary should always be avoided following the principle of least privilege.

Examples

packages/snap/snap.manifest.json:L69

"endowment:network-access": {},

Recommendation

Remove superfluous permissions.

Description

Metamask by default protects wallet addresses from being exposed to connected websites. A user wishing to expose a wallet address to a dapp must explicitly connect that address with the dapp website. Here is an example of MetaMask Flask with a random test wallet managing 2 accounts. Account 1 is connected to metamask, test2 is not.

The dapp can query for connected addresses via the MetaMask injected provider RPC method eth_requestAccounts. Other non-connected addresses will not be returned:

await window.ethereum.request({ method: 'eth_requestAccounts' })
['0x3d0c4e58b3ff2516455f79c1147eb95f125d56ae']

In contrast, the shapeshift snap requests low-level access for the ethereum root key. The snap exposes a similar RPC endpoint named eth_getAddress that returns all ethereum addresses. Any connected dapp can query the snap to retrieve ethereum addresses. The dapp - not necessarily trusted - can even silently interact with the snap to enumerate all ethereum addresses, whether they’re ‘connected’ to the dapp or not. This effectively bypasses MetaMask security measures where the user defines the addresses to expose.

Via the ShapeShift snap eth_getAddress RPC endpoint, the dapp can effectively enumerate all addresses even though they’re not connected via the main wallet. This circumvents MetaMask security measures undermining established security principles of the wallet.

Note that all *_getAddress RPC endpoints exhibit this problem.

Examples

• MetaMask APO only exposes connected addresses

await window.ethereum.request({ method: 'eth_requestAccounts' })
['0x3d0c4e58b3ff2516455f79c1147eb95f125d56ae']

• the same address can be retrieved via the ShapeShift snap RPC API

await window.ethereum.request({
    method: 'wallet_invokeSnap',
    params: {
      snapId: "local:http://localhost:9000",
      request: { method: 'eth_getAddress', params: {addressParams:{addressNList: [0x80000000 + 44, 0x80000000 + 60, 0x80000000 + 0, 0, 0]}, snapId: "local:http://localhost:9000" }},
    }});
'0x3D0C4e58b3fF2516455f79c1147eB95F125d56aE'

• MetaMask does not expose other addresses. However, the snap RPC API allows to enumerate non-connected addresses too, undermining the security of the main wallet.

await window.ethereum.request({
    method: 'wallet_invokeSnap',
    params: {
      snapId: "local:http://localhost:9000",
      request: { method: 'eth_getAddress', params: {addressParams:{addressNList: [0x80000000 + 44, 0x80000000 + 60, 0x80000000 + 0, 0, 1]}, snapId: "local:http://localhost:9000" }},
    }});
'0xBaB66CfA59757200c90c79BC6e2aEe4bFBe382Be'

Recommendation

Ensure that the implemented security measures match those of the main wallet. Allow users to choose which addresses they want to expose to the dapp. Do not give low-level access to all wallet addresses without user consent.

Description

The signing request message does not display the user account used to sign the message. A malicious dapp may pretend to sign a message with one account while issuing an RPC call for a different account.

ShapeShift snap signing requests should implement similar security measures to how MetaMask signing requests work. Being fully transparent on “who signs what”, and displaying the origin of the request. This is especially important on multi-dapp snaps to avoid users being tricked into signing transactions they did not intend to sign (wrong signer; dapp race condition).

Please note that we have also reported to the MM Snaps team that dialogs do not, by default, hint at the origin of the action. We hope this will be addressed commonly for all snaps in the future.

Recommendation

Display the signing account in a human-readable and expected format on the signing request. Also, display the origin of the RPC call.

Description

On certain occasions, the snap may need to present a dialog to the user to request confirmation for an action or data verification. This step is crucial as dapps are not always trusted, and it’s essential to prevent scenarios where they can silently sign data or perform critical operations using the user’s keys without explicit permission. To create custom user-facing dialogs, MetaMask provides the Snaps UI package, equipped with style-specific components. However, some of these components have been found to have unintended side-effects.

For instance, the text() component can render Markdown or allow for control character injections. Specifically for the ShapeShift snap, this poses a concern because when the snap asks the user to sign structured data, that data might be mistakenly interpreted as Markdown. As a result, the user could inadvertently sign something they did not intend to sign. This means that if the message-to-be-signed contains Markdown renderable text, the displayed message for user approval will be inaccurate.

In the code snippet provided below, please note that the variable params is considered potentially untrusted. It may contain Markdown renderable strings or Control Characters that can disrupt the context of the user-displayed message.

Examples

• UserConfirm does not sanitize params

await window.ethereum.request({
    method: 'wallet_invokeSnap',
    params: {
      snapId: "local:http://localhost:9000",
      request: { method: 'binance_signTransaction', params: {transaction:{"hi **bold**\n\nnextline **test**":1}}},
    }});

packages/snap/src/rpc/common/utils.ts:L89-L110

export const userConfirm = async (params: userConfirmParam): Promise<boolean> => {
  try {
    /* eslint-disable-next-line no-undef */
    const ret = await snap.request({
      method: 'snap_dialog',
      params: {
        type: 'confirmation',
        content: panel([
          heading(`${params.prompt}: ${params.description}`),
          text(params.textAreaContent),
        ]),
      },
    })
    if (!ret) {
      return false
    }
  } catch (error) {
    moduleLogger.error(error, { fn: 'userConfirm' }, 'Could not display confirmation dialog')
    return false
  }
  return true
}

packages/snap/src/rpc/common/BaseSigner.ts:L54-L60

protected async confirmTransaction(transaction: any): Promise<boolean> {
  return await userConfirm({
    prompt: `Sign ${this.coin} Transaction?`,
    description: 'Please verify the transaction data below',
    textAreaContent: JSON.stringify(transaction, null, 2),
  })
}

• SnapDialog (not referenced anywhere but potentially vulnerable)

packages/adapter/src/metamask/metamask.ts:L114-L140

/**
 * TODO: This is a snap-native call - a handler must be added to the snap onRpcRequest() method to support this.
 */
export const snapDialog = async ({
  prompt,
  description,
  textAreaContent,
}: {
  prompt: string
  description: string
  textAreaContent: string
}): Promise<boolean> => {
  const provider = await getMetaMaskProvider()
  if (provider === undefined) {
    throw new Error('Could not get MetaMask provider')
  }
  if (provider.request === undefined) {
    throw new Error('MetaMask provider does not define a .request() method')
  }
  try {
    const ret = await provider.request({
      method: 'snap_dialog',
      params: {
        type: 'confirmation',
        content: panel([heading(`${prompt}: ${description}`), text(textAreaContent)]),
      },
    })

Please note that we have also reported the need for plaintext UI elements to the MM Snaps team. We hope this will be addressed commonly for all snaps in the future.

Recommendation

Validate inputs. Encode data in a safe way to be displayed to the user. Show the original data provided within a pre-text or code block. Show derived or decoded information (token recipient) as additional information to the user.

Description

The codebase currently lacks inline documentation, and the repository is missing high-level documentation explaining the Snap capabilities and features. This absence of documentation poses several concerns for future maintenance and transparency. Without inline documentation, as the codebase grows, understanding the code’s logic and functionality can be more challenging for developers, making maintenance and bug fixes more time-consuming and error-prone. Additionally, the absence of high-level documentation makes grasping the Snap’s intended functionality and capabilities hard for end-users.

Recommendation

We recommend adding inline documentation throughout the codebase to facilitate comprehension of the code’s behavior and contribute to its maintainability. We also recommend adding comprehensive high-level documentation in the repository, detailing the Snap’s capabilities, features, and intended usage. This will offer insights to developers and end-users, promoting transparency for all parties.

Description

MetaMask core is set to Ethereum as the default network. When switching to BNB or other Networks, Metamask asks the user to confirm the switch. This ensures that, at any point, the user is fully aware of the network they are currently operating on.

ShapeShift Snap exports multi-chain functionality, making it available to connected dapps via the MetaMask RPC. Connected dapps can request operations on various chains without requiring the users to confirm a chain switch. This deviates from the MetaMask security principles of always keeping the user informed about chain switches. Furthermore, the user does not have fine-grained control over what chain functionality is exposed to the dapp.

For example, since there is no origin check in the RPC handler onRpcRequest(), any connected dapp may access ShapeShift snap functionality. Some dapps may only require access to Avalanche or Thorchain-related functionality, while others may request access to functionality for several chains. Following the principle of least privilege, the user should be able to choose the chains dapps can access instead of granting access to every chain as soon as the dapp is connected to the snap. Indeed, this behavior poses a substantial phishing risk.

Recommendation

We recommend keeping an internal state of the last chain used. When a dapp requests to access functionality for a different chain, ask the user to confirm the chain switch. Give users control over what chains they want to expose to the dapp and keep a record of their choice. For example, the first time a dapp access Avalanche-specific features, the user should be able to accept or reject the dapp from accessing the network. Incorporate MetaMask’s security measures without compromising or weakening them in any way.

Description

The transaction signing process lacks essential information to make sense of the transaction data object. The addressNList is assumed to be a BIP-32 path without proper explanation, and the contained information is presented in a non-human-readable format. As a result, the user cannot easily identify critical information, such as the signer’s address. This leads to a non-user-friendly experience, which also poses security concerns.

Recommendation

Provide some means for the user to understand what they are signing. Display the signing request origin (multi-dapp usage). Additionally, show the raw data they’re actually signing. Decode the BIP-32 key path to a user-readable address.

Description

The current logic to get the Ethereum provider implemented in getMetaMaskProvider does not have any timeout to wait for provider initialization. If this logic is used in the Snap initialization code, because of the asynchronous nature of the initialization code, the function might return an “undefined” provider because the provider is not yet initialized. This would throw an error and prevent the detection of the installed Metamask version. It would be better to provide a safe timeout and wait before deciding whether the provider is undefined.
Alternatively, one could rely on the @metamask/detect-provider package (which is already part of the project dependencies) to get the provider.

packages/snap/src/rpc/common/utils.ts:L112-L119

const getMetaMaskProvider = async (): Promise<ExternalProvider> => {
  try {
    // eslint-disable-next-line no-undef
    const provider = (window as any).ethereum
    assert(provider, 'Could not detect Ethereum provider')
    return provider
  } catch (error) {
    moduleLogger.error(

Recommendation

Rely on the @metamask/detect-provider package to get the Ethereum provider or implement a timeout before deciding whether the provider is undefined.

Description

The AVAX functionality is enabled in the snap but disabled in the sandbox dapp. This is problematic as the functionality is present in the snap but not documented or surfaced anywhere.

packages/sandbox/src/components/AssetCardList/AssetCardListConfig.ts:L65-L69

name: 'Avalanche',
icon: 'avax.png',
symbol: 'AVAX',
enabled: false,
actions: {

packages/snap/src/index.ts:L88-L100

export const onRpcRequest: OnRpcRequestHandler = async ({ request }) => {
  const { method, params } = request
  switch (method) {
    case 'avax_getAddress':
      return await avalancheGetAddress(params)
    case 'avax_signMessage':
      return await avalancheSignMessage(params)
    case 'avax_signTransaction':
      return await avalancheSignTransaction(params)
    case 'avax_verifyMessage':
      return await avalancheVerifyMessage(params)
    case 'avax_broadcastTransaction':
      return await ethereumBroadcastTransaction(params)

Recommentation

Similarly to other networks, surface the AVAX functionality in the sandbox dapp.

Description

The sandbox dapp returns an error when calling eth_signMessage:

{
  "code": -32603,
  "message": "Cannot read properties of undefined (reading 'map')",
  "data": {
    "originalError": {}
  }
}

Recommendation

Fix the default message to be signed. Include the from address/HD-path in the signing parameters.

Description

The interface type RPCRequest is declared but not referenced in the codebase.

Examples

packages/snap/src/index.ts:L83-L94

interface RPCRequest {
  origin: string
  request: ShapeShiftSnapRPCRequest
}

export const onRpcRequest: OnRpcRequestHandler = async ({ request }) => {
  const { method, params } = request
  switch (method) {
    case 'avax_getAddress':
      return await avalancheGetAddress(params)
    case 'avax_signMessage':
      return await avalancheSignMessage(params)

Recommendation

async ({ request } : RPCRequest ) => {

Description

Every RPC request to the Snap follows the same pattern: it goes through a “handler” that creates a new signer for the proper chain, initializes it, and returns the result of the signer’s operation. As an example, here is the handler for the cosmos_getAddress RPC request:

packages/snap/src/rpc/cosmossdk/cosmos/handlers.ts:L17-L28

export const cosmosGetAddress = async (
  params: CosmosGetAddressParams,
): Promise<CosmosGetAddressResponse> => {
  try {
    const cosmosSigner = new CosmosSigner()
    await cosmosSigner.initialize()
    return await cosmosSigner.getAddress(params)
  } catch (error) {
    moduleLogger.error({ fn: 'cosmosGetAddress' }, error)
    return Promise.reject(error)
  }
}

The signer is initialized by deriving the keys from the Metamask BIP32 entropy (in this.initializeSigner()) and creating a new unchained HTTP client for the appropriate RPC endpoint.

packages/snap/src/rpc/cosmossdk/cosmos/CosmosSigner.ts:L31-L46

async initialize(
  { broadcastUrl }: SignerInitializeArgs = {
    broadcastUrl: broadcastUrls.DEFAULT_UNCHAINED_COSMOS_HTTP_URL,
  },
) {
  const httpProviderConfiguration = new unchained.cosmos.Configuration({
    basePath: broadcastUrl,
  })
  try {
    this.signer = await this.initializeSigner()
    this.httpProvider = new unchained.cosmos.V1Api(httpProviderConfiguration)
    this._initialized = true
  } catch (error) {
    this.logger.error(error, { fn: 'getSigner' }, `Failed to initialize ${this.coin}Signer`)
  }
}

While this pattern works, it is sub-optimal performance-wise as the Snap re-creates new signer objects for every RPC request. Assuming RPC requests are generally sequential, e.g., get_address -> sign_tx -> broadcast_tx, this might lead to the creation/garbage collection of many objects with potentially expensive operations (key derivation, for instance). In that case, using the Singleton pattern would likely help as it would prevent creating a new signer to process every RPC request. The presence of the initialization pattern with the “initialized” flag in the signer classes also indicates that the initial engineer’s intentions were likely to use such a pattern. Yet, this flag is not used in the current codebase.

Recommendation

While the current code does not pose a problem from a security standpoint, we would recommend leveraging the Singleton pattern for the signer classes. This would prevent creating new signer objects for every RPC request but only a single instance of the signer class for the first request. It would positively impact the performance of the Snap when dealing with many RPC requests.

Description

snapDialog logs a misleading error message if confirmation times out. This seems to be a copy-past error from the walletSnap method.

packages/adapter/src/metamask/metamask.ts:L142-L147

} catch (error) {
  /** User did not confirm the action or an error was encountered */
  moduleLogger.error(error, { fn: 'walletSnap' }, `wallet_snap_* RPC call failed.`)

  return Promise.reject(error)
}

packages/adapter/src/metamask/metamask.ts:L107-L111

  return ret
} catch (error) {
  moduleLogger.error(error, { fn: 'walletSnap' }, `wallet_snap_* RPC call failed.`)
  return Promise.reject(error)
}

Recommendation

Log an accurate error message.