Opium Smart Contracts Security Analysis

Ivan Ivanitskiy
SmartDec Cybersecurity Blog
10 min readMar 24, 2020

--

In this report, we consider the security of the Opium project. Our task is to find and describe security issues in the smart contracts of the platform.

Disclaimer

The audit does not give any warranties on the security of the code. One audit cannot be considered enough. We always recommend proceeding with several independent audits and a public bug bounty program to ensure the security of smart contracts. Besides, security audit is not an investment advice.

Summary

In this report, we considered the security of Opium smart contracts. The description of the project can be found here. We performed our audit according to the procedure described below. The audit showed no critical issues. However, three medium severity and a number of low severity issues were found. We discussed them with the developers, so in the latest version of the code they fixed most of the issues found in the audit and provided their comments on the rest of the issues.

General recommendations

The contracts code is of good code quality. Thus, we do not have any additional recommendations.

Procedure

In our audit, we consider the following crucial features of the smart contract code:

  1. Whether the code is secure.
  2. Whether the code corresponds to the documentation (including whitepaper).
  3. Whether the code meets best practices in efficient use of gas, code readability, etc.

We perform our audit according to the following procedure.

Automated analysis

  • we scan project’s smart contracts with our own Solidity static code analyzer SmartCheck
  • we scan project’s smart contracts with several publicly available automated Solidity analysis tools such as Ethlint and Solhint
  • we manually verify (reject or confirm) all the issues found by tools

Manual audit

  • we manually analyze smart contracts for security vulnerabilities
  • we check smart contracts logic and compare it with the one described in the documentation
  • we run tests

Report

  • we reflect all the gathered information in the report

Checked vulnerabilities

We have scanned Ethex smart contracts for commonly known and more specific vulnerabilities. Here are some of the commonly known vulnerabilities that we considered (the full list includes them but is not limited to them):

Project overview

Project description

In our analysis we consider Opium specification (”docs/index.md” in the project repository) and smart contracts’ code (private repository, version on commit 60ff6f80996b83f6ad19c35b74480fef34f7dc03). Also, the main dependency — erc721o (version on commit 60723b713e3f3f9d98d71b800673e0b4696fe108) was analysed.

The latest version of the code

After the initial audit, some fixes were applied and the code was updated to the latest version (commit 8c7d076fa0ee958a82134c32412d972452746dff).

Project architecture

For the audit, we were provided with the truffle project. The project is an npm package and includes tests.

  • The project successfully compiles with truffle compile command (with some warnings, see Compilation output in Appendix)
  • The project successfully passes all the tests, however, code coverage was not generated

The total LOC of audited Solidity sources is 1569.

Manual analysis

The contracts were completely manually analyzed, their logic was checked and compared with the one described in the documentation. Besides, the results of the automated analysis were manually verified. All the confirmed issues are described below.

Critical issues

Critical issues seriously endanger smart contracts security. We highly recommend fixing them.

The audit showed no critical issues.

Medium severity issues

Medium issues can influence smart contracts operation in current implementation. We highly recommend addressing them.

Overpowered role

The governor address is responsible for whitelisting addresses in TokenSpender contract. That means that governor address is responsible for Opium Token transfers.

So, the system depends heavily on governor address in the current implementation. In this case, there are scenarios that may lead to undesirable consequences for investors, e.g. if the governor’s private keys become compromised. Thus, we recommend designing contracts in a trustless manner.

Comment from the developers: ”TokenSpender contract is used to hold user’s allowances on ERC20 contracts and allows only whitelisted contracts to spend tokens. For purpose of users security we introduced “time locks”, which notify users when content of whitelist is about to change and doesn’t allow to change it before specific moment in future, so users could verify newly proposed whitelist and decide whether they want to keep their allowance or not.”

Bug

There is a bug in SwaprateMatchBase.sol file, line 62. address(0) is used instead of _token address to transfer tokens:

ERC20(address(0)).transfer(msg.sender, balance);

We recommend fixing this bug (i.e. calling _token address).

The issue has been fixed and is not present in the latest version of the code.

Deployment process

There is no deployment script in the project. However, the contracts deployment does not seem trivial. Bugs and vulnerabilities often appear in deployment scripts and severely endanger system’s security.

We highly recommend developing and testing deployment scripts very carefully.

The issue has been fixed and is not present in the latest version of the code.

Moreover, there are a lot of setter-functions in Registry.sol (lines 44–89).

We recommend moving the complexity of the project deployment and initialization from smart contracts to deployment scripts in order to reduce gas costs, and removing set***() functions from ABI, as they will be used only once. Bounded smart contracts can be deployed as described in this article.

Comment from the developers: ”We didn’t implement bounding contracts like described in your article, but eliminated all setter to one init() function in Registry contract.”

Low severity issues

Low severity issues can influence smart contracts operation in future versions of code. We recommend taking them into account.

Compiler version

Solidity source files indicate the versions of the compiler they can be compiled with. Example:

pragma solidity ^0.5.4; // bad: compiles w 0.5.4 and above
pragma solidity 0.5.4; //good: compiles w 0.5.4 only

We recommend following the latter example, as future compiler versions may handle certain language constructions in a way the developers have not foreseen. Besides, we recommend using the latest compiler version — 0.5.16 at the moment.

The issue has been fixed and is not present in the latest version of the code.

Gas usage

Gas usage may be optimized in several places in the project:

  • WhitelistedWithGovernance.sol, line 53 whitelist variable is used as the Committed event argument. We recommend using _whitelist variable instead.

The issue has been fixed and is not present in the latest version of the code.

  • WhitelistedWithGovernance.sol, line 59 proposedWhitelist variable is used as the Proposed event argument. We recommend using _whitelist variable instead.

The issue has been fixed and is not present in the latest version of the code.

  • Core.sol, line 521 registry.getOpiumAddress() is called multiple times. We recommend saving it to the local memory after the first call. The same issue arises at MatchLogic.sol, lines 159, 166.

The issues have been fixed and are not present in the latest version of the code.

  • MatchLogic.sol, line 151 registry.getTokenSpender() is called multiple times. We recommend saving it to the local memory after the first call.

The issue has been fixed and is not present in the latest version of the code.

  • Whitelisted.sol, line 14 whitelist.length is read in a loop. We recommend saving it to the local memory.

The issue has been fixed and is not present in the latest version of the code.

  • Whitelisted.sol, line 9 onlyWhitelisted() modifier consumes a lot of gas due to lots of storage read operations. Keep in mind, that the price of reading storage variables can be changed in the future. For example, it was increased 4 times in the Istanbul hardfork.

Code logic

  • WhitelistedWithGovernance.sol, line 50 initialized variable is used to check whether whitelist has been initialized or not. We recommend using whitelist.length instead.

The issue has been fixed and is not present in the latest version of the code.

  • SyntheticAggregator.sol, line 119 _derivativeHash is a function argument. However, hash of the derivative is calculated again. We recommend not passing it as a function’s argument and just calculating it instead.

Comment from the developers: ”In places, where this function is called internally we consider _derivativeHash as trusted and use it to retrieve Ticker state without recalculating the hash and optimise gas usage.”

  • MatchSwap.sol, lines 137, 143 When calculating order’s filled percentage, 1 is added to every calculated value. We recommend removing these additions.

The issue has been fixed and is not present in the latest version of the code.

Missing input validation

There are input validation issues in several places:

  • WhitelistedWithGovernance.sol, line 81 In case _governor argument is zero address, whitelisting functionality will be lost.
  • Registry.sol, line 86 In case _opiumAddress argument is zero address, fees will be locked.

We recommend adding check that _governor and _opiumAddress arguments are non-zero.

The issues have been fixed and are not present in the latest version of the code.

Redundant code

The following lines are redundant:

  • WhitelistedWithGovernanceAndChangeableTimelock.sol, line 13
uint256 timelockProposalTime = 0;
  • WhitelistedWithGovernanceAndChangeableTimelock.sol, line 15
uint256 proposedTimelock = 0;
  • WhitelistedWithGovernance.sol, line 22
bool public initialized = false;
  • WhitelistedWithGovernance.sol, line 25
uint256 public proposalTime = 0;

At these lines, the initial values are reassigned.

We highly recommend removing redundant code in order to improve code readability and transparency and decrease cost of deployment.

The issues have been fixed and are not present in the latest version of the code.

Visibility level

registry variable of usingRegistry contract has private visibility level without getter function. This may affect user experience in a negative way as users cannot easily check the registry address.

The issue has been fixed and is not present in the latest version of the code.

Implicit visibility level

The following variables have an implicit visibility level:

  • ExecutableByThirdParty.sol, line 6
mapping (address => bool) thirdpartyExecutionAllowance;
  • LibCommission.sol, line 6
uint256 constant COMMISSION_BASE = 10000;
  • LibCommission.sol, line 9
uint256 constant OPIUM_COMMISSION_BASE = 10;
  • LibCommission.sol, line 12
uint256 constant OPIUM_COMMISSION_PART = 1;
  • WhitelistedWithGovernanceAndChangableTimelock.sol, line 13
uint256 timelockProposalTime = 0;
  • WhitelistedWithGovernanceAndChangableTimelock.sol, line 15
uint256 proposedTimelock = 0;
  • SwaprateMatch.sol, line 17
mapping (bytes32 => uint256) filled;
  • SwaprateMatchBase.sol, line 31
mapping (bytes32 => bool) canceled;
  • SwaprateMatchBase.sol, line 36
mapping (bytes32 => bool) verified;
  • SwaprateMatchBase.sol, line 44
mapping (address => mapping (address => uint256)) public balances;

We recommend specifying visibility levels (public, private, or internal) explicitly and correctly in order to improve code readability.

The issues have been fixed and are not present in the latest version of the code.

Misleading name

validateCanceled() function of SwaprateMatchBase contract checks whether the order is not canceled.

We recommend renaming it to validateNotCanceled().

The issue has been fixed and is not present in the latest version of the code.

Transfer of an unknown ERC20 token

Return value of transfer() function is ignored in the following places:

  • MatchLogic.sol, line 70
  • Core.sol, line 64
  • SwaprateMatchBase.sol, line 62

According to ERC20 token standard:

The function SHOULD throw if the message caller’s account balance does not have enough tokens to spend.

Since the code of this token contract is unknown, transfer() function call may not revert in the case of unsuccessful token transfer. We recommend using SafeERC20 contract from OpenZeppelin library.

The issues have been fixed and are not present in the latest version of the code.

Code duplications

There are code duplications in the contracts, for example at MatchSwap.sol, lines 133–137 and 139–143.

We recommend refactoring duplicated parts of code.

Comment from the developers: ”We consider refactoring not feasible, thus we agreed to keep it as is.”

Constant state variable

commission variable at HasCommission.sol, line 8 is never changed.

We recommend adding constant keyword and naming it in UPPER_CASE.

The issue has been fixed and is not present in the latest version of the code.

Prefer external to public visibility level

In the code, there are functions with the public visibility level that are not called internally.

We recommend changing visibility level of such functions to external in order to improve code readability. Moreover, in many cases functions with external visibility modifier require less gas comparing to functions with public visibility modifier.

Comment from the developers: ”We consider refactoring not feasible, thus we agreed to keep it as is in most of the places where issue occurs.”

Notes

Naming convention

  • WhitelistedWithGovernance.sol, line 16 TIME_LOCK_INTERVAL state variable is not constant, but is named in UPPER_CASE_WITH_UNDERSCORES.
    We recommend naming it in mixedCase.
  • usingRegistryErrors.sol, line 3 usingRegistryErrors contract is named in mixedCase.
    We recommend naming it in CapWords.
  • usingRegistry.sol, line 8 usingRegistry contract is named in mixedCase.
    We recommend naming it in CapWords.

The issues have been fixed and are not present in the latest version of the code.

Gas limit

The assign operations at WhitelistedWithGovernance.sol, lines 52, 58, and 72 are costly as the whole arrays are being copied. If there are too many items in these arrays, the execution of the corresponding functions will fail due to an out-of-gas exception.

Comment from the developers: ”By conducting tests we consider refactoring not feasible, thus we agreed to keep it as is.”

Avoid using experimental features in production code

ABIEncoderV2 is used in the code. We do not recommend using experimental features in the code since they might contain bugs that will only be fixed in future versions of the compiler.

Comment from the developers: ”We are widely using passing structures as function arguments, thus ABIEncoderV2 is required for us and we can’t avoid it.”

Gas limit and loops

The following loops traverse through arrays of variable length:

  • Core.sol, line 300
for (uint256 i; i < tokenIds.length; i++)
  • Core.sol, line 344
for (uint256 i; i < tokenIds.length; i++)

The traversed arrays are passed as parameters of the functions. Therefore, if there are too many items in these arrays, the execution of the corresponding functions will fail due to an out-of-gas exception.

In these cases, we recommend separating the calls into several transactions.

This audit was performed by SmartDec, a security team specialized in static code analysis, decompilation and secure development.

Feel free to use SmartCheck, our smart contract security tool for Solidity and Vyper, and follow us on Medium, Telegram and Twitter. We are also available for smart contract development and auditing work.

--

--

Product Manager at PARSIQ. Co-host at Basic Block podcast. Bitcoin, Ethereum, InfoSec. Libertarianism, MMA, IPSC practitioner.