Metamask Snap Audit Report for XRPL Snap

Management Summary

XRPL Snap contacted Sayfer Security in order to perform penetration testing on XRPL’s MetaMask Snap in January 2024.

Before assessing the above services, we held a kickoff meeting with the XRPL Snap technical team and received an overview of the system and the goals for this research.

Over the research period of 2 weeks, we discovered 7 vulnerabilities in the system.

In conclusion, several fixes should be implemented following the report, but the system’s security posture is competent.

After a review by the Sayfer team, we certify that all the security issues mentioned in this report have been addressed by the XRPL Snap team.

Risk Methodology

At Sayfer, we are committed to delivering the highest quality penetration testing to our clients. That’s why we have implemented a comprehensive risk assessment model to evaluate the severity of our findings and provide our clients with the best possible recommendations for mitigation.

Our risk assessment model is based on two key factors: IMPACT and LIKELIHOOD. Impact refers to the potential harm that could result from an issue, such as financial loss, reputational damage, or a non-operational system. Likelihood refers to the probability that an issue will occur, taking into account factors such as the complexity of the attack and the number of potential attackers.

By combining these two factors, we can create a comprehensive understanding of the risk posed by a particular issue and provide our clients with a clear and actionable assessment of the severity of the issue. This approach allows us to prioritize our recommendations and ensure that our clients receive the best possible advice on how to protect their business.

Risk is defined as follows:

Security Evaluation Methodology

Sayfer uses OWASP WSTG as our technical standard when reviewing web applications. After gaining a thorough understanding of the system we decided which OWASP tests are required to evaluate the system.

Security Assessment

After understanding and defining the scope, performing threat modeling, and evaluating the correct tests required in order to fully check the application for security flaws, we performed our security assessment.

Issue Table Description

Issue title

Description

Here we provide a brief description of the issue and how it formed, the steps we made to find or exploit it, along with proof of concept (if present), and how this issue can affect the product or its users.

Mitigation

Suggested resolving options for this issue and links to advised sites for further remediation.

Security Assessment Findings

[M] SignHelper(string, any) Fails to Return an Error

Description

SignHelper(string, any) calls on TransactionRisk(string), in order to verify via the offchain API whether the transaction’s destination has suspicious history. Based on this, a certain classification is returned, which is then returned to the user on the screen. If this operation returns an unspecified error, the SignHelper function will catch it with a try-catch statement. However, the catch block does not perform any action, so this error will be ignored and the user will receive unplanned output on the screen.

• SignHelper(string, any)

  try {
    xrplorerTransactionRisk = await SignTransaction.TransactionRisk(
      payment.Destination,
    );
  } catch (err) {}

Mitigation

In the event of an error on the API side, a message should be displayed to the user, or conversely the transaction should be aborted altogether.

[M] Dysfunctional Hardcoded Domain

Description

As part of the risk classification process, a certain hardcoded URL is queried. During our testing attempts were made to verify several sample addresses in order to analyze the behavior of the API, but each time, it returned a code 500 error. Therefore, it was not possible to verify whether the risk classification process is working correctly. Additionally, there is no way to change this domain to another one (without modifying the code) because it is hardcoded.

• TransactionRisk(string)

  private static TransactionRisk(address: string) {
    return fetch(
    https://europe-west3-xrpl-snap-366820.cloudfunctions.net/proxy-to-xrpl-forensics?address=${address}`,
      {
        method: 'GET',
        mode: 'cors',
        redirect: 'follow',
      },
    ).then((response) => response.json());
  }

Mitigation

We suggest you verify why the server is not working properly. Additionally, we recommend adding an option to configure the domain on which the API operates, as well as checking its behavior for various token addresses.

[L] No-Null Assertions Can Be Avoided

Description

Description In several cases, the snap uses non-null assertions, which are considered a bad security practice. A more stringent approach is to use the optional chaining operator (?), which automatically checks whether the element before it is `null` or `undefined`. If this is the case, the entire expression returns undefined, rather than throwing an error when we try to reference a property or call a method on null or undefined values.

Mitigation

Mitigation We recommend changing the implementation and implementing the chaining operator (?) instead of non-null assertions (!).

[L] Test scenarios and unit tests are missing

Description

The snap has specification tests for each of the files in src/handler and src/transactions, and several in src/xrpl. However, they do not fully cover all functionalities and features found in the codebase.

Moreover, there is a lack of complete and accurate end-to-end tests and verification of various test scenarios, such as signing and sending transactions with different inputs.

Mitigation

We suggest covering the entire code with tests to ensure that it has been implemented in accordance with the assumed business and functional logic.

[I] Leftover TODOs

Description

Description During our research, we found three instances of TODO comments scattered throughout the snap code.

Mitigation

Mitigation We suggest that you resolve all comments marked as TODO, or use an internal, non-public list of places where the code is not finished in production.

[I] Non-functioning Demo

Description

Description When trying to install Snap in the default configuration, an error is returned indicating that version 0.1.0 is not compatible with the expected version 0.3.2. While this can be easily rectified, new and especially non-technical users may find this confusing.

Mitigation

Mitigation We suggest customizing your Snap configuration so that it runs by default after you build and install it.

[I] Unimplemented Demo Methods

Description

Two methods in the demo are not actually supported by the code. These are “Transaction Info” and “Get NFTs”. There are no handlers that could handle such functionalities.

export const getAccountNfts = async (address?: string) => {
console.log('get NFTs', address);

return await window.ethereum.request({
method: 'wallet_invokeSnap',
params: {
snapId: SNAP_ENDPOINT,
request: {
method: 'xrpl_accountNfts',
params: { address },
}
},
});
};

export const getTransactionInfo = async (transactionHash?: string) => {
console.log('get transaction info', transactionHash, SNAP_ENDPOINT);
return await window.ethereum.request({
method: 'wallet_invokeSnap',
params: {
snapId: SNAP_ENDPOINT,
request: {
method: 'xrpl_transactionInfo',
params: { transactionHash }
}
}
});
};

Mitigation

Mitigation We recommend either removing these functionalities from the demo or conversely, actually implementing them.