Web3 Tickets

Description

The POST endpoint called by the create_ticket function does not implement any access control mechanism nor does not seem to implement any kind of DoS protection. An attacker can create tickets for any arbitrary address by providing the corresponding address_key, even if he does not own the corresponding private key. That is made possible because the backend does not check whether the API key is authorized for the address key that is submitted.

Examples

packages/snap/utils/backend_functions.ts:L5-L18

export async function create_ticket(req_address: string, title: any, description: any, apikey: any) {

    const address_key = SHA256(req_address, { outputLength: 32 }).toString();

    try{
        const requester = "[email protected]";
        const ticketData = {ticket: {
            subject: title,
            requester: requester,
            tags: 'anon_' + address_key,
            comment:{body: description }
        }};
        const url = 'https://71z6182pq3.execute-api.eu-west-1.amazonaws.com/default/tickets?create=true';
        fetch(url, {

Recommendation

When new tickets are submitted, the backend must check that the API key is authorized to create new tickets for that particular address key. Additionally, one should consider implementing proper rate limiting or equivalent DoS protection to prevent the mass creation of tickets by a single user or address, mitigating the risk of support flooding.

Description

Anyone can enumerate all tickets belonging to any wallet address by providing the corresponding address key (SHA256 of the address) to the right API endpoint. Users can also create tickets on behalf of other users by sending a POST request to the API with the corresponding address key. Finally, any user can view and post updates to tickets belonging to other users by injecting arbitrary ticket IDs in GET/POST requests sent to the API. The root cause is that the ticket IDs and address keys are not authenticated against a particular user ID/access token. Thus, it allows unauthorized access to potentially sensitive ticket information and the ability to impersonate users and create tickets and post comments to tickets belonging to other users. This is a critical flaw which must be fixed.

Examples

packages/snap/utils/backend_functions.ts:L5-L17

export async function create_ticket(req_address: string, title: any, description: any, apikey: any) {

    const address_key = SHA256(req_address, { outputLength: 32 }).toString();

    try{
        const requester = "[email protected]";
        const ticketData = {ticket: {
            subject: title,
            requester: requester,
            tags: 'anon_' + address_key,
            comment:{body: description }
        }};
        const url = 'https://71z6182pq3.execute-api.eu-west-1.amazonaws.com/default/tickets?create=true';

packages/snap/utils/backend_functions.ts:L38-L42

const address_key = SHA256(req_address, { outputLength: 32 }).toString();

try{
    const url = 'https://71z6182pq3.execute-api.eu-west-1.amazonaws.com/default/tickets';
    const final_url = url + '?address=' + address_key;

packages/snap/utils/backend_functions.ts:L67-L72

export async function get_ticket_comments(ticket_id : any, req_address: any, apikey: any){
    let json_ticket_comments = null;
    const address_key = SHA256(req_address, { outputLength: 32 }).toString();

    try{
        const url = 'https://71z6182pq3.execute-api.eu-west-1.amazonaws.com/default/tickets?ticketId=' + ticket_id;

packages/snap/utils/backend_functions.ts:L94-L100

export async function update_ticket(ticket_id: any, input_data: any, req_address: any, apikey: any) {
    console.log(`Updating ticket ${ticket_id} with comment: `, input_data);
    const address_key = SHA256(req_address, { outputLength: 32 }).toString();

    try {
        const url = 'https://71z6182pq3.execute-api.eu-west-1.amazonaws.com/default/tickets?ticketId='
            + ticket_id + '&create=false' + '&from_snap';

Recommendation

Implement strict validation and proper access control mechanisms to ensure that users can only create, view and modify tickets they are authorized to access. This could involve verifying user permissions against the ticket ID and address key before granting access or allowing modifications.

Description

Currently, there is no validation of the origin for RPC calls made to the Snap. Thus, any dapp connected to the Snap can call any RPC endpoint, which allows an attacker crafting a malicious dapp to access a user’s tickets and impersonate the user to support services. The vulnerability arises because the RPC call handler onRpcRequest() can be invoked by any dapp as it does not validate the origin of the call. This enables any connected dapp to call get_snap_state, exposing critical user information like its userID, API token, and ticket details.

Examples

packages/snap/src/index.ts:L242-L247

export const onRpcRequest: OnRpcRequestHandler = async ({ request }) => {
 
  switch (request.method) {
   
    case 'set_snap_state':
      let allTicketCommentsCount: any = [];

Recommendation

Implement strict origin validation in the onRpcRequest() handler to ensure that only authorized and intended domains can invoke the Snap. Additionally, consider removing the get_snap_state RPC endpoint if it is not strictly needed.

Description

Currently, if multiple tickets are updated (have new comments), the Snap will only trigger a notification for the last updated ticket. The fetchAllTicketCommentsCount function only triggers a notification for the last updated ticket, as the variable updatedTicketId gets overridden in each iteration of the loop by the last updated ticket id. Consequently, updates to other tickets are not notified. This issue is problematic for users who have multiple tickets opened concurrently, as when there are multiple tickets with new comments, all except the last updated ticket go unnoticed.

Examples

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

for(let i = 1; i < ticket_ids_and_comment_count.length; i += 3){
  if(ticket_ids_and_comment_count[i] > allTicketCommentsCount[i]){
    if(ticket_ids_and_comment_count[i+1] == 'agent'){
      if (ticket_ids_and_comment_count[i - 1] == allTicketCommentsCount[i - 1]) {
        fireAlerts = true;
        updatedTicketId = ticket_ids_and_comment_count[i - 1];
        console.log("There is an update on ticket ID: ", updatedTicketId);
        console.log('Firing notifications...');
        setSnapState(apiKey, address, allTicketCommentsCount, dialog);

Recommendation

Revise the notification logic to ensure that the user is notified in case multiple tickets are updated. One could accumulate all updated ticket IDs in an array and then iterate over this array to fire alerts for each updated ticket. In case there are many updated tickets, one should probably notify the user and redirect him to the dapp to read the new messages.

Description

The Snap’s current design, which pulls all user tickets and comments from the server every second, poses significant performance and efficiency issues. This constant fetching process occurs for every user who has installed the Snap, leading to concerns, particularly for users with numerous tickets and comments. The Snaps has to re-process the same data repeatedly, despite having a local cache. This approach not only renders the local caching inefficient, as data is re-fetched in every cycle, but also places an unnecessary load on the backend and results in the same data being transmitted many times.

Additionally, it can also be highlighted that the snaps contain unnecessary state updates: every key in the Snap’s state is updated every time, even if it hasn’t changed. For instance, but not exclusively, here:

packages/snap/src/index.ts:L91

setSnapState(apiKey, address, allTicketCommentsCount, dialog);

packages/snap/src/index.ts:L103

setSnapState(apiKey, address, allTicketCommentsCount, dialog);

Recommendation

We recommend redesigning the data fetching mechanism to enhance efficiency and performance. One should aim to fetch only new or updated comments, potentially by cleverly leveraging ticket and comment IDs/timestamps. This would reduce the load on both the client and the server and minimize unneeded computing and data transfer. If tickets can be updated, one also needs to consider this case. In general, one should also avoid unnecessary Snap state updates by updating only the modified keys.

Description

Snaps UI components are prone to markdown and control character injections. In this case, one of the risk is the potential for operators to mislead users. For example, an operator could create a comment that falsely appears as if another person has joined the discussion. This is achieved by formatting a comment with a reply followed by a separator and then falsely attributing a message to someone else, as in <officialReply>\n\n______________________\n\n<OtherPerson>: <OtherComment>.

Considering users should only be able to see their own and the operator’s comments, and operators are assumed non-malicious, this vulnerability should not pose significant security concerns. However, one should keep that in mind in case security assumptions evolve.

Examples

packages/snap/src/index.ts:L120-L123

for (let i = 0; i < json.length; i++){
  const comment = json[i]['body'];
  const sender = json[i]['via']['channel'] == 'api' ? '**You**' : '**Agent**'
  formatted_comments += `${sender}: ${comment}\n\n______________________\n\n`;

Recommendation

As a mitigation, one could implement comprehensive content sanitization and strict validation for markdown rendered in UI components to prevent such misleading representations. Alternatively, one could investigate rendering comments in distinct UI components.

Description

The current fetchAllTicketCommentsCount() function is overly complex and poorly optimized. For instance, the function unnecessarily exhibits a sequence of “then” promises, which could be avoided. Moreover, the function fetches ticket comments sequentially, whereas a parallel approach would be more efficient. It also parses ticket_count from the received JSON data and uses it to iterate over that data, yet it fails to validate that ticket_count is less than the length of the received data. These examples illustrate that it would be a good idea to refactor and optimize the function to increase the readability and robustness of its code.

We would suggest optimizing the function and simplifying its code to improve readability and robustness. More precisely, one could make the following changes:

• Exit early if variables like address or apiKey are not initialized
• Sanitize and validate relevant untrusted values, such as the ticket_count
• Refactor the data processing logic to be more succinct and readable. Consider using different data structures (e.g., objects) to simplify the logic and improve the robustness of the code.
• Leverage parallel data fetching by utilizing Promise.all() to concurrently fetch ticket comments
• Streamline the code by removing unnecessary chains of promises, logging, and verbose comments
• Add proper error handling

packages/snap/src/index.ts:L35-L42

const fetchAllTicketCommentsCount = async () => {

  let allTicketCommentsCount : any = [];
  let fireAlerts = false;
  let updatedTicketId = null;

  const state = await getSnapState();
  const address = state?.address as string

packages/snap/src/index.ts:L53-L77

await get_user_tickets(address, apiKey)
 .then( async (json : any) => {
   const ticket_count = json['count'];
   if (ticket_count > 0)
     for(let i = 0; i < ticket_count; i++){
       const ticket_id = json['rows'][i]['ticket']['id'];
       await get_ticket_comments(ticket_id, address, apiKey).then( (json : any) => {
         if(json.length > 0){
           const last_comment_source = json[json.length-1]['via']['channel'];
           ticket_ids_and_comment_count.push(ticket_id);
           ticket_ids_and_comment_count.push(json.length);
           if(last_comment_source == 'api')
             ticket_ids_and_comment_count.push('user');
           else if (last_comment_source == 'web')
             ticket_ids_and_comment_count.push('agent');
         }
       })
     }
 })
 .then(() => {
   // console.log(ticket_ids_and_comment_count);
   // console.log(allTicketCommentsCount);
     if(allTicketCommentsCount.length == 0){
     console.log('Notification process started.');
   }

packages/snap/src/index.ts:L86-L99

            if (ticket_ids_and_comment_count[i - 1] == allTicketCommentsCount[i - 1]) {
              fireAlerts = true;
              updatedTicketId = ticket_ids_and_comment_count[i - 1];
              console.log("There is an update on ticket ID: ", updatedTicketId);
              console.log('Firing notifications...');
              setSnapState(apiKey, address, allTicketCommentsCount, dialog);
            }
          }
        }
      }
    }
  }
}).then(() =>{
  // console.log(ticket_ids_and_comment_count);

Recommendation

We recommend completely refactoring the fetchAllTicketCommentsCount() function in line with these suggestions. This refactoring will not only make the code more readable and maintainable but also improve its performance.

As a future improvement, one should also investigate more optimized mechanisms to fetch tickets, such as server-side push instead of client-side pull or smarter algorithms to fetch only new comments. This would significantly improve user performance with many tickets/comments and reduce both server and client-side load. (see issue 4.5 )

Description

Currently, if the Snap’s state is empty or undefined, fetchAllTicketCommentsCount() tries to fetch tickets for the key corresponding to the hash of the empty string "". This occurs when the snap is installed and the state has not yet been initialized by the dapp, for instance. This leads to unnecessary processing on the server and could also result in more severe issues. In general, functions should validate and sanitize any input parameters, especially if they are not trusted. In this case, fetchAllTicketCommentsCount(), updateTicket(), and parseTicketComments() should exit early if any of their parameters or values retrieved from the state, such as apiKey, address, or ticketID are not defined.

Examples

packages/snap/src/index.ts:L35-L44

const fetchAllTicketCommentsCount = async () => {

  let allTicketCommentsCount : any = [];
  let fireAlerts = false;
  let updatedTicketId = null;

  const state = await getSnapState();
  const address = state?.address as string
  const apiKey = state?.apiKey as string
  const dialog = state?.dialog as string

packages/snap/src/index.ts:L133-L139

async function updateTicket(ticketId: any, user_comment: any) {
  // need a function here as getSnapState() doesn't seem to work in cron
  const state = await getSnapState();
  const address = state?.address as string
  const apiKey = state?.apiKey as string

  const update_result = await update_ticket(ticketId, user_comment, address, apiKey);

packages/snap/src/index.ts:L113-L117

async function parseTicketComments(ticketId: any) {
  const state = await getSnapState();
  const address = state?.address as string
  const apiKey = state?.apiKey as string
  let formatted_comments = `Login to https://web3tickets.metamask.io to see your personal dashboard with all tickets open for your ethereum account address. \n\n`;

Recommendation

Modify fetchAllTicketCommentsCount(), updateTicket(), and parseTicketComments() to include early exit conditions. These functions should immediately return or exit if essential parameters such as apiKey, address, or ticketID are not defined. Implementing these checks will prevent the functions from executing with incomplete or invalid data.

Description

The codebase currently exhibits a lack of consistency in coding style: inconsistent use of semi-colons, inconsistent line lengths, and other stylistic discrepancies. These inconsistencies make the codebase harder to read and maintain, potentially leading to future issues. Similarly, some typescript annotations are tautologies and should be simplified. Unneeded return statements should also be ditched.

The code also exhibits patterns that are generally considered unsafe, e.g., the use of an “if” without brackets, or loose equality (==) instead of strict equality (===). These patterns should be removed in favor of safer patterns.

Additionally, the codebase contains unused imports, resulting in unnecessary complexity. It also appears to transform the bundle (in snaps.config.js.bundlerCustomizer()) by appending the Buffer package, which does not seem to be used in the Snap.

Finally, for conciseness and clarity, one should remove unneeded code and statements, such as unreachable code.

Example

Inconsistent use of semi-colons:

packages/snap/src/index.ts:L135-L137

const state = await getSnapState();
const address = state?.address as string
const apiKey = state?.apiKey as string

The typescript annotation allTicketCommentsCount: any | [] can be simplified to any:

packages/snap/src/index.ts:L16-L16

const setSnapState = async (apiKey: string | null, address: string | null, allTicketCommentsCount: any | [], dialog : string | null) => {

Unsafe if statement:

packages/snap/src/index.ts:L56-L57

if (ticket_count > 0)
  for(let i = 0; i < ticket_count; i++){

Weak equality checks:

packages/snap/src/index.ts:L75-L76

if(allTicketCommentsCount.length == 0){
console.log('Notification process started.');

Unreachable code:

packages/snap/src/index.ts:L229-L230

throw new Error('Method not found.');
break;

This always return true or throws an error. The return statement can safely be removed:

packages/snap/src/index.ts:L259-L260

await setSnapState(request.params.apiKey, request.params.address, allTicketCommentsCount, request.params.dialog);
return true;

Some extra imports:

packages/snap/src/index.ts:L2-L3

import { panel, text, heading, divider } from '@metamask/snaps-ui';
import { create_ticket, get_user_tickets, get_ticket_comments, update_ticket } from '../utils/backend_functions';

Note that these are just a few examples but more instances of some of these patterns are present in the codebase.

Recommendation

Implement the use of a linter to enforce a consistent coding style throughout the codebase. This will not only improve the readability and maintainability of the code but also help to prevent potential bugs that can arise from stylistic inconsistencies. Refactor unsafe code patterns, particularly ensuring that ‘if’ statements and similar constructs are used with brackets to prevent potential bugs. Remove any unused imports and unneeded code segments, such as unreachable code, to enhance the codebase’s clarity, safety, and reduce bundle size.

Description

The codebase lacks inline documentation. This reduces the maintainability of the codebase as it can make it challenging for new developers and auditors to understand the code as it grows. Proper inline documentation is essential for maintaining code clarity and future maintainability.

Recommendation

We recommend adding inline documentation throughout the codebase. This should include comments explaining the purpose and functionality of key functions. The documentation should be clear, concise, and consistent to ensure it is helpful and understandable for anyone reviewing or working with the code.

Description

The codebase exhibits inconsistent use of naming conventions, alternating between camel case and underscore naming. In general, it is better to avoid mixing conventions to increase the code’s readability and maintainability. Additionally, there are instances where function and variable names are misleading, which can cause confusion about their purpose and functionality. For example, a function named fetchAllTicketCommentsCount misleadingly suggests it returns a count, while it actually returns a boolean and a ticket ID.

Examples

packages/snap/src/index.ts:L35-L36

const fetchAllTicketCommentsCount = async () => {

packages/snap/src/index.ts:L52

let ticket_ids_and_comment_count: any[] = [];

packages/snap/src/index.ts:L37

let allTicketCommentsCount : any = [];

Recommendation

Standardize the naming conventions, preferably choosing camel case according to Javascript standard practices. Additionally, review and rename functions and variables to accurately reflect their purpose and output. This will enhance code clarity and significantly improve the auditability and maintainability of the codebase.