Metamask Snap Audit Report for Massa

Management Summary

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

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

Custom Networks Won’t Function Correctly

Description

The snap implements two default networks for which the logic is customized – MAINNET and BUILDNET. The default is the latter, while functions, such as getClient(bigint), check for both options.

• src/accounts/clients.ts:23-30

return await ClientFactory.createDefaultClient(
  chain Ξ CHAIN_ID.MainNet
      ? DefaultProviderUrls.MAINNET
      : DefaultProviderUrls.BUILDNET,
  chain,
  true,
  account,
);

However, the snap also allows adding custom networks, via setNetwork(), which saves the provided network ID to the state.

• src/hooks/useSetNetwork.ts:20-27

export const setNetwork: Handler<SetNetworkParams, SetNetworkResponse>
= async (
  params,
) =€ {
  const network = coerceParams(params);
 
  await setActiveChainId(network);
  return { network: network.toString() };
};

Unfortunately, the rest of the codebase does not support setting custom networks. getClient(bigint) and getNodeUrls() only support the two default networks. Consequently, if, for example, the selected chain is TESTNET or LABNET, DefaultProviderUrls() will return BUILDNET.

Mitigation

We recommend implementing another mechanism for selecting the default provider. If the selected chain is different from MAINNET and BUILDNET, the user should be able to set a custom provider.

Incorrect Return Value

Description

getBalance(GetBalanceParams) returns two values, finalBalance and candidateBalance, but both variables are assigned the same value.

• src/handlers/get-balance.ts:48-51

return {
  finalBalance: balance Ycandidate YtoString() '0',
  candidateBalance: balance Ycandidate YtoString() '0',
};

Mitigation

Assuming that returning two variables was not a mistake, they should be assigned different values.

Duplicate Operations Are Not Saved

Description

The checks whether there is no identical operation in state according to the operation:address key. If an object is detected, the operation is skipped.

• src/operations.ts:34-3G

if (operations.find((t) =€ t Ξ operation)) {
  return;
}

This can be an issue, since information about certain operations, such as transfers, is valuable even if they are performed many times.

Mitigation

Consider removing the check and allowing duplicate operations to be saved to memory.

Roll Operations Are Not Saved to Storage

Description

Unlike transfers, roll operations are not saved to memory. The operation is returned to the caller function after execution, but nowhere in the logic is the operation added to storage.

Mitigation

buyRolls(BuyRollsParams) and sellRolls(SellRollsParams) should call addAccountOperation(string, string) in order to save their operations to memory.

Imprecise Method Names

Description

The codebase contains two methods that, while their names suggest that they return a plurality of objects, actually return a single one.

• listAccounts(), which returns an account
• getNodeUrls(), which returns a single URL.

Mitigation

Adjust the function names to reflect the reality of the code.

No Parameter Prefix Validation

Description

Several comments in the codebase (attached to the functions specified above) specify that addresses and operation IDs have different prefixes – addresses are prefixed with AU, while operation IDs are prefixed with OP – this never comes to play in the code itself.

Mitigation

Consider adding checks that validate the prefixes of the supplied strings, if this was your intention when writing the comments.

Function May Fail If the Number of Operations is Too Large

Description

getOperations() returns all operations stored in memory. Since an operation is added to the array every time a unique action is performed on the wallet, it is possible (if unlikely) that the number of operations grows too large if the snap is continuously and actively used for a long enough period of time.

Mitigation

Consider introducing a limit to the number of operations returned through getOperations() or to the number of operations stored in memory. If the limit is exceeded, older operations could be deleted.