Metamask Snap Audit Report for Qtum

Management Summary

Qtum contacted Sayfer Security in order to perform penetration testing on Qtum’s MetaMask Snap in 05/2024.

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

Over the research period of 2 weeks, we discovered 5 vulnerabilities in the system, none of them critical.

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 Qtum 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

No Method to Remove Added Networks

Description

The code implements the ability to add additional Networks, extending their list beyond the default one. However, it was noted that there is no way to remove them except by reinstalling the snap. This functionality was probably planned, because there is a removeNetwork() function, but it is not possible to call it from publicly available methods.

export const networks = {
    get: getNetworks,
    setCurrent: setCurrentNetwork,
    add: addNetwork,
    remove: removeNetwork,
};

Mitigation

We recommend implementing this method, as was probably intended.

Missing Confirmation Prompt for Exporting Private Keys

Description

In most critical functions in the snap, a dialog box, which informs the user about the action being performed and requests their approval, is displayed.

However, this safety measure is not present in the WalletExportPrivateKey method, the one used to display the private key on the snap screen. If invoked by mistake, it would immediately display the private key.

Mitigation

Before displaying the private key, add a confirmation prompt requesting the user’s approval.

Multiple Unimplemented Methods

Description

A huge portion of methods present in the snap appear to be left unused. Only the boilerplate surrounding them exists, but not any concrete logic.

Mitigation

We recommend reviewing the methods you would like to implement in your snap and making sure that they all contain actual logic. It should not be possible for the user to call unnecessary methods that you do not intend to implement.

Redundant Code

Description

During our analysis, we have noticed that two RPC methods, EthRequestAccounts and EthAccounts, appear to do the same thing and return the same values.

case RPCMethods.EthRequestAccounts: {
    const wallet = await getWallet();
    return [wallet.address];
}
case RPCMethods.EthAccounts: {
    console.log('RPCMethods.EthAccounts');
    console.log('request.params', JSON.stringify(request.params));
    const wallet = await getWallet();
    return [wallet.address];
}

In both cases, these methods retrieve the active wallet’s data by using getWallet() and then return its address. Currently, the only difference is that EthAccounts logs while EthRequestAccounts doesn’t.

Mitigation

Consider either removing one of the methods or modifying one to differentiate it from the other

Leftover TODO Comments

Description

The codebase contains several TODO comments and also sections of commented out code that is no longer used.

While this is acceptable in a test environment, it should be eliminated in production. Such comments may provide a potential stakeholder with valuable information about missing functionalities, security measures and developers’ thoughts, which can be used to profile attacks.

Mitigation

We suggest removing comments marked as TODO and unused code fragments in your production branch.