MetaMask Snap Audit Report For Pianity

Management Summary

Pianity contacted Sayfer Security in order to perform a security audit on Pianity’s MetaMask Snap in July 2023.

Before assessing the above services, we held a kickoff meeting with the Pianity 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.

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

Arsnap/Adapter

Usage Of Vulnerable Packages

Description

An pnpm audit command has revealed that the package semver is considered vulnerable to a regex denial of service attack. See more here.

Mitigation

It’s advisable to upgrade the package to 6.3.1 or later, as this vulnerability was already fixed.

Low Log Limit

Description

The logsStorageLimit variable will work only for a few usages of snap – 100 entries except is_enabled and get_permissions methods are only allowed. Above 100, the oldest entry will be silently overwritten without using the return value from logs.splice().

registerLog:

if (logs.unshift(entry) > logsStorageLimit) {
logs.splice(logsStorageLimit, logs.length - logsStorageLimit);
}

Mitigation

Considering the size of the logs as text data, a better solution is to increase their allowable size, for example to 1000.

Unhelpful Error Message

Description

getLogInfo(), supports all available public methods as part of the log logging process. If the `method` variable does not find its equivalent in switch, it goes to default, i.e. the exhaustive function is returned. However, this function returns a non-descriptive error message, which may be misleading.

● exhaustive():

export function exhaustive(_: never): never {
throw new Error("Check wasn't exhaustive");
}

Mitigation

The error message should be made more specific.

Misleading Return Value

Description

requestPermissions(), used to handle requests for new permissions for the dApp from the user, verifies whether the requested permissions are not already granted. If this is true for all requested permissions, newPermissions is of length zero, as no new permission will be granted. However, even when no new permissions have been granted, the value of the granted parameter is returned as true.

requestPermissions():

if (newPermissions.length === 0) {
return { granted: true, permissions: currentPermissions };
}

Mitigation

One possibility is to change granted to something other than a boolean. An enum with three values is the most obvious choice: one value for true, a second for false, and a neutral third, that means that no new permissions have been granted, but nothing was denied.

TODOs in the Code

Description

We found a few cases of left-over TODOs around the codebase.
Occurrences:
– packages/adapter/src/const.ts:1
– packages/adapter/src/const.ts:5
– packages/arsnap/src/handlers.ts:196
– packages/arsnap/src/state.ts:50

Mitigation

Simply remove the comments.

Usage of Deprecated Functions

Description

It has been noticed that functions marked as deprecated are used in several places in Snap.

Affected functions include:

• atob(),
  packages/arsnap/src/crypto.ts:45
  packages/arsnap/src/utils.ts:23
• btoa()
  packages/arsnap/src/utils.ts:18

Mitigation

Please refer to this Stack Overflow thread for more information.

Vault

Missing Summary Field in CreateOperation

Description

The logical.CreateOperation path in KeyPath is not implementing the Summary field like the rest of the operations within OperationHandler.

Notice how most operations implement a Summary field, but CreateOperation does not:

Operations: map[logical.Operation]framework.OperationHandler{
logical.ReadOperation: &framework.PathOperation{
Callback: handleRead,
Summary: "Retrieve the secret from the map.",
},
logical.UpdateOperation: &framework.PathOperation{
Callback: handleWrite,
Summary: "Store a secret at the specified location.",
},
logical.CreateOperation: &framework.PathOperation{
Callback: handleWrite,
},
logical.DeleteOperation: &framework.PathOperation{
Callback: handleDelete,
Summary: "Deletes the secret at the specified location.",
},
},

Mitigation

Add the missing Summary field.

Appendix A: Security Evaluation Fixes

After a review by the Sayfer team, we certify that all the above-mentioned security issues have been addressed by the Pianity team.

Fixes commit: c8b9fc2ca579cabf422b6bf122314e9378fa2658