Skip links
Briken logo

Briken Audit

Introduction

CoinFabrik was asked to audit the contracts for the Briken project. First, we will provide a summary of our discoveries and then, we will show the details of our findings.

Summary

The contracts audited are from the Briken code package with file MD5 hashes detailed below:

Briken.sol 8e3d0723dba40fa1e8d863e8d7df013a
Exchange.sol 7be82308695c4c87b0f789ed04e54397
FibraFactory.sol 9e5299a602cda2304262186a3e6b3c7a
Fibra.sol bf964a440ab4a6e21508befb1675e78f
Migrations.sol f5528c8f92c865439deb7f2a70cb8d58
Notary.sol fe53550c6855f88f9d9e193cb8cb23a3
OurMultiSigWallet.sol fd3649687b7251f184cbb368fd11a194
OurRoles.sol 809f909687b65fac258dae9c8822d33c
RBAC.sol 18cda146f08961b332468716be5f4419
WalletOperations.sol b727e8d426132937b46a6839573ed72b
WhiteListing.sol 9997d9978b6309f28e71b683dfce529f

 

Contracts

The audited contracts are:

  • Briken.sol: Briken ERC20 Token.
  • Exchange.sol: Allows the exchange between Fibras and Briken.
  • Fibra.sol: Fibra ERC20 Token.
  • FibraFactory.sol: Allows the creation of Fibra contracts.
  • Notary.sol: Emits notarization events.
  • OurMultiSigWallet.sol: Gnosis’ MultiSigWallet multisignature wallet.
  • OurRoles.sol: Defines roles and modifiers for role-based access control.
  • RBAC.sol: Getters and setters for defining roles used in access control.
  • WalletOperations.sol: Wallet operations such as making and taking sell orders.
  • WhiteListing.sol: More role-related functionality.

Analyses

The following analyses were performed:

  • Misuse of the different call methods
  • Integer overflow errors
  • Division by zero errors
  • Outdated version of Solidity compiler
  • Front running attacks
  • Reentrancy attacks
  • Misuse of block timestamps
  • Softlock denial of service attacks
  • Functions with excessive gas cost
  • Missing or misused function qualifiers
  • Needlessly complex code and contract interactions
  • Poor or nonexistent error handling
  • Failure to use a withdrawal pattern
  • Insufficient validation of the input parameters
  • Incorrect handling of cryptographic signatures

Detailed findings

Severity Classification

Security risks are classified as follows:

  • Critical: These are issues that we manage to exploit. They compromise the system seriously. They must be fixed immediately.
  • Medium: These are potentially exploitable issues. Even though we did not manage to exploit them or their impact is not clear, they might represent a security risk in the near future. We suggest fixing them as soon as possible.
  • Minor: These issues represent problems that are relatively small or difficult to take advantage of but can be exploited in combination with other issues. These kinds of issues do not block deployments in production environments. They should be taken into account and be fixed when possible.
  • Enhancement: These kinds of findings do not represent a security risk. They are best practices that we suggest to implement.

 

This classification is summarized in the following table:

SEVERITY EXPLOITABLE ROADBLOCK TO BE FIXED
Critical Yes Yes Immediately
Medium In the near future Yes As soon as possible
Minor Unlikely No Eventually
Enhancement No No Eventually

Issues Found by Severity

Critical severity

No issues of critical severity have been found.

Medium severity

No issues of medium severity have been found.

Minor severity

makeSellOrder does not protect against spam orders

The makeSellOrder function allows anyone to create sell orders.

A malicious user with the balance of just 1 Fibra could create multiple sell orders and pollute the mapping of existing orders, which would only incur in gas costs for the user. There’s no limit or validation that prevents the user from doing so.

The contract’s owner has no capabilities of deleting unwanted sell orders.

Moreover, most of these orders would be useless since the order’s maker might only have 1 Fibra token available to exchange and therefore would only be capable of fulfilling a single sell order.

This could become a problem in the database or frontend if they get filled with meaningless information.

Additionally, the following require statement in the function is redundant:

require(fibraAmount <= IERC20(fibraAddress).allowance(msg.sender, this));

Since the allowance is already checked when doing the transferFrom in takeSellOrder, and the allowance is not decreased until the sell order is taken.

takeSellOrder considers 0 a valid sellPrice

In the function takeSellOrder from WalletOperations.sol the following require statement can be seen:

( , , , ,uint256 sellPrice) = exchange.orders(orderId);
require(sellPrice >= 0, “Order id doesn’t exist”);

Where orders is a mapping of the struct Order.

However, in the definition of Order we can see that a value of 0 in the sellPrice field is considered invalid, therefore this require statement would let an Order through which is not considered valid and could be uninitialized.

Additionally, the check is completely redundant because an unsigned int can never be smaller than 0 by definition.

Remember that in Solidity, accessing a position in a mapping which has not been previously assigned does not throw an error, and in this case checking for a value greater than 0 is necessary to make sure that the orderId passed to the function exists.

Therefore the code should look like this:

( , , , ,uint256 sellPrice) = exchange.orders(orderId);
require(sellPrice > 0, “Order id doesn’t exist”);

Note that now we have a strict inequality and thus the value of 0 is no longer considered valid and will trigger a revert.

Enhancements

Avoid floating version pragmas

Right now the following pragma is used throughout the project to specify Solidity’s compiler version:

pragma solidity >=0.4.21 <0.6.0;

This allows accidentally deploying the contracts with a different compiler version than the one we tested or intended to deploy the contracts with.

We strongly recommend choosing a specific Solidity version and using a fixed pragma, for example, if you wish to use the 0.5.17 version which is the last in the 0.5.x series, the pragma would be as follows:

pragma solidity 0.5.17;

Unnecessary OurMultiSigWallet contract

Currently a contract named OurMultiSigWallet can be found, that inherits from Gnosis’ MultiSigWallet.

However, OurMultiSigWallet does not add nor modify anything from the parent contract, currently it just calls the parent constructor, therefore it is not necessary. The project could use MultiSigWallet directly if no further modifications are intended.

Roles could be defined as enum for better performance

The RBAC contract is used for role-based access control throughout the project, in which the following clarification is made:
“This RBAC method uses strings for key roles. It may be beneficial  to write your own implementation of this interface using Enums or similar.”

Switching to using enums as keys instead of strings would bring two benefits: slightly reducing the gas usage every time the mapping is accessed and enforcing which values can be used as keys, because currently even if roles are defined as constants, an arbitrary string parameter could be passed accidentally to one of these functions.

For example adding the following in OurRoles.sol instead of the already defined string constants:

enum Roles {Signer, Data_Provider, Beneficiary}

Please note that this will require changes specially in RBAC.sol and other contracts since we are now dealing with values in this enum instead of strings.

Additionally, the modifiers defined in OurRoles.sol are by default empty, which would cause problems if a programmer accidentally decides to use them without overriding first with proper checks, since by default they allow the function to continue; a good way to avoid this is to make OurRoles an Interface instead of a Contract: which enforces the programmer to override the modifier when using it.

Allowance is not decreased when cancelling an order

Currently, in the Exchange contract, in order to execute a sell order, the Fibra token must have an allowance for the Exchange contract on behalf of the order’s maker.

The allowance would disappear in the call to transferFrom when the order is taken by a buyer, however, if the order is never taken and the maker decides to cancel it by calling cancelSellOrder this allowance is never decreased, and the Exchange contract will have the power to transfer tokens on behalf of the previously existing order’s maker, even if this order does not exist anymore.

Conclusion

We found the contracts to have some minor quirks that overall aren’t exploitable, but contribute to making the code harder to read and maintain. We would advise correcting these issues before deploying, but they are not critical.

Disclaimer: This audit report is not a security warranty, investment advice, or an approval of the Briken project since CoinFabrik has not reviewed its platform. Moreover, it does not provide a smart contract code faultlessness guarantee.
This website uses cookies to improve your web experience.