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.
