General testing methodology

The scope for this audit was sufficiently small to focus heavily on manual review of the code. Two potential impacts have been outlined as the main focus for this test, these being the ability for a user to have unintended full control over assets in the custodial wallet and the ability for a user to trick the return system into thinking an asset has been returned, while still being in possession of the asset.

With these goals in mind, Zellic tried to identify general web vulnerabilities as well as logical bugs that would allow a user to achieve that. Given below are a few examples of tests and potential logical bugs that Zellic investigated; this list is not comprehensive.

Query tampering

As the returning of assets involves a database, the most immediate way an attacker could potentially trick the application into thinking an asset has been returned is by tampering with the queries.

While attacker-controlled input is passed into the queries, it is done in a safe way. For example,

class ReturnOrderItemsController {
  async create(req, res) {
    const { userId } = req?.auth?.isAdmin ? req.params : req.auth;
    const { rentalOrderItemIds = [] } = req.body;
    (...)
    const returnableAssets = await orderService.fetchReturnableAssets(userId, rentalOrderItemIds)
    (...)
  }
}

Source: packages/rest-apis/wallet-api/src/app/controllers/ReturnOrderItemsController.ts

here the user has partial control over the userId and full control over the rentalOrderItemIds. These are then passed to the orderService.fetchReturnableAssets() function.

async fetchReturnableAssets(userId: string, rentalOrderItemIds: string[]) {
    const itemsToReturn = await lootrushDbReplica(
      'marketplace.rental_orders_items as roi'
    )
      .select(
        'roi.id',
        (...)
      )
      (...)
      .where({
        userId,
      })
      .whereIn('roi.id', rentalOrderItemIds)
      .whereNot('roi.status', RentalOrderItemStatus.RETURNED);
    if (itemsToReturn.length > 0) {
      return itemsToReturn;
    }

    return [];
  }

Source: packages/rest-apis/wallet-api/src/app/services/RentalOrderService.ts

Here the user-controlled input is passed into the SQL queries in a safe manner by using the Knex query builder. The same pattern is used across the audited code. If, for example, the input would have been passed into Knex's .raw() query, a user may be able to manipulate the input in a way that would result in running arbitrary SQL commands, tricking the return logic.

Asset returns

Another potential way a user could trick the system is by returning someone elses assets. This type of logical bug is prevented in multiple ways.

First, the JWT uniquely identifies the user and checks if the user is an admin. A middleware is used to verify the JWT before every controller method invocation. This ensures that the JWT which the user has provided has not been tampered with.

When it is then accessed in the controller, a check is performed using the JWT if the user is an admin. If the user is not an admin, only values that are contained inside the JWT are used for processing, preventing a malicious actor from passing arbitrary values to functions.

class ReturnOrderItemsController {
  async create(req, res) {
    const { userId } = req?.auth?.isAdmin ? req.params : req.auth;
    (...)
  }
}

Source: packages/rest-apis/wallet-api/src/app/controllers/ReturnOrderItemsController.ts

Further, every action that is taken is done so using the userId, which is always constructed using the JWT. This ensures that actions are always for the intended user.

class ReturnOrderItemsController {
  async create(req, res) {
    const { userId } = req?.auth?.isAdmin ? req.params : req.auth;
    (...)
    const returnableAssets = await orderService.fetchReturnableAssets(userId, rentalOrderItemIds)
    (...)
    const wallet = await custodialWalletService.find({ userId }, false);
    const cryptoWallet = await custodialWalletService.getCryptoWalletInstance(wallet)
    const result = await orderService.returnAssets(returnableAssets, cryptoWallet);
    return res.status(200).send({ result })
  }
}

Source: packages/rest-apis/wallet-api/src/app/controllers/ReturnOrderItemsController.ts

It should be noted that while Zellic did not identify issues with the handling of JWTs inside the audited code, interactions or handling of JWTs outside the audited code may impact the behavior in unintended ways.

Method firewall

In combination with the above mentioned mitigations, the code also makes use of a method firewall, which restricts the methods a user is allowed to call for their custodial wallet.

When a user requests to call a method, the request is routed through the controller, which checks the JWT, as demonstrated above, to make sure that the user does not call methods on other wallets but their own. The request then ends up in the callMethod handler.

async callMethod(
    wallet: UserCustodialWallet,
    method: string,
    params
  ): Promise<string | boolean> {
    const cryptoWallet = await this.getCryptoWalletInstance(wallet);
    const isMethodAllowedFN = methodFirewall[method] || methodFirewall.default;
    const isMethodAllowed = await isMethodAllowedFN(params);
    const [walletCall] = await lootrushDb<UserCustodialWalletCall>(
      'user_custodial_wallet_calls'
    )
      .insert({
        id: uuid(),
        userCustodialWalletId: wallet.id,
        method,
        params: JSON.stringify(params),
        response: JSON.stringify({ isMethodAllowed }),
      })
      .returning('*');

    // verify if transaction can procceed
    if (!isMethodAllowed) return '';

    const response = await cryptoWallet.callMethod(method, params);
    await lootrushDb<UserCustodialWalletCall>('user_custodial_wallet_calls')
      .where({
        id: walletCall.id,
      })
      .update({
        response: JSON.stringify(response),
      });

    return response;
  }

Source: packages/rest-apis/wallet-api/src/app/services/CustodialWalletService.ts

Here the user's wallet is retrieved, the method that has been passed is checked against the allowlist, and then it is either rejected or executed depending on if the method was in the allowlist. During the audit, Zellic did not discover any instances where an attacker could circumvent the logic implemented in the audited code.