Securitize Smart Contract Audit

Introduction

CoinFabrik was asked to audit the Securitize contracts for the DSToken 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 DSToken repository at https://bitbucket.org/securitize_dev/dstoken. The audit is based on commit b9e74fc84f562b12bb06d3c30d29be32507886e0, and updated to reflect changes at commit d41228237979845cdbdc83e216bd44903a0de472.

Description

DSToken is an implementation of Securitize’s Digital Securities Protocol. It is composed of several modules that combined together create a security token.

• Compliance: Contains regulatory policies that can be applied to a security token. 
• Registry: Consists of a registry of investors and regulatory information.
• Trust: Implements several Roles and privileges that can interact with a token.
• Token: An ERC-20 token implementation that can be used as a security token.
• Utils: Miscellaneous utilities
  • Proxy contract that  deploys lightweight versions of other contracts using the pattern delegate proxy.

The graph below is a high level representation of the most relevant system modules. 

Note: Solid lines indicate inheritance and dotted lines indicate composition.

We have audited two previous versions of the contracts. One important change in this version consists in replacing the “Eternal Storage” pattern with a simpler storage strategy. 

Analyses

We checked the code for the following errors and attacks:

• 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 have been found.

Medium severity

No issues have been found.

Minor severity

Implementation error in MutiSigWallet’s getTransactionCount

The function getTransactionCount in MultiSigWallet.sol counts transactions that are in a determinate state.

The examined implementation fails to iterate over all the transactions. It does not lead to a security issue by itself but its erroneous behavior might cause an application to fail to properly account for transactions. Besides, its correct behavior is hard to emulate using other functions.

Adding braces to properly iterate all transactions would fix the incorrect behavior.

function getTransactionCount(bool pending, bool executed) public view returns (uint256 count) {
    string memory transactionId;

    for (uint256 i = 0; i < transactionCount; i++) {
        transactionId = countedTransactionIds[i];;
        if ((pending && !transactions[transactionId].executed) || (executed && transactions[transactionId].executed)) {
            count += 1;
        }
    }
}

Fixes were applied at commit 3caf2fa7..41f4.

Enhancements

Modification of array’s length

In the function removeOwner from MultiSigWallet.sol the length of the array is modified directly.

This issue has no security implications. We recommend using owners.pop() instead, since it is compatible with future versions of solidity compiler.

function removeOwner(address owner) public onlyWallet ownerExists(owner) {
    isOwner[owner] = false;
    for (uint256 i = 0; i < owners.length - 1; i++)
        if (owners[i] == owner) {
            owners[i] = owners[owners.length - 1];
            break;
        }
    owners.pop();
    if (required > owners.length) changeRequirement(owners.length);
    emit OwnerRemoval(owner);
}

Fixes were applied at commit 3caf2fa7..41f4.

Modifier named onlyFromProxy does not reflect its functionality

ProxyTarget contract defines the modifier onlyFromProxy.  The modifier name implies that the function is called from a Proxy. This is not accurate.

modifier onlyFromProxy() {
     require(__t1 != address(0x0), "Must be initialized from proxy");
    _;
 }

The condition requires the variable ___t1 (or target when called from a Proxy) to be initialized. It does not have a security implication directly involved but its incorrect usage may cause problems in the future. We suggest to use a more suitable name like onlyFromInitializedProxy since it is only used by initialize functions.

Suggested fix was applied at commit 3caf2fa7..41f4.

Boolean parameter with unclear functionality 

In several functions, boolean parameters are used and the caller utilizes the values true and false which, without their proper context, might be confusing and induce bugs, such as the following call to updateInvestorBalance:

updateInvestorBalance(_tokenData, registryService, _to, _value, true);

At a first glance, without going to the function definition, we can’t tell whether this increments or decrements the investor balance.

Compared to the previous code, this alternative is less ambiguous:

enum BalanceOperation { Increase, Decrease }

updateInvestorBalance(_tokenData, registryService, _to, _value, BalanceOperation.Increase);

This avoids potential bugs if a programmer ever confuses true/false when calling the mentioned functions, which might cause unintended behaviours.

We suggest replacing these hardcoded boolean values with enums, making sure to write the necessary changes to the functions’s signatures and body.

A fix was applied at commit 3caf2fa7..41f4.

Code for checking if a string is empty can be refactored

In many contracts, the following code (or a similar one) for checking if a string is empty is used:

if (keccak256(abi.encodePacked(str)) != keccak256("")) { […] }

This could be easily refactored into a separate function which would improve readability and reduce code complexity:

function strIsEmpty(string memory _str) pure returns(bool) {
     return keccak256(abi.encodePacked(_str)) == keccak256("");
 }
 if (!strIsEmtpy(str)) { […] }

A new function called isEmptyString() was added at commit 3caf2fa7..41f4.

Observations

Unnecessary Interfaces

There are many contracts in the project behaving as interfaces, providing type definitions and function signatures, these separate files are not necessary when just a single contract provides the implementation.

We suggest merging both the interfaces and the actual contract into one file to simplify the complexity of the codebase.

Among these are included:

• IDSServiceConsumer and ServiceConsumer
• IDSTrustService and TrustService
• IDSTokenIssuer and TokenIssuer

Update the latest compiler pragmas

Currently every file has the following pragma: pragma solidity ^0.5.0. Using a floating pragma version has the disadvantage that any compiler from 0.5 can be used, including future releases that might change a minor detail affecting the contracts or past a version with a known bug.

We suggest changing it to use the fixed version pragma solidity 0.5.17 as to enforce the use of the latest available Solidity compiler in the 0.5.x versions, and to not depend on the environment where it will be compiled.

Solidity version was set to version 0.5.17 at commit 3caf2fa7..41f4.

Missing error messages in require

The require() function has an optional parameter which allows to provide a message error which will be shown to the user when the condition fails.

In most of the cases, this message is not provided, we recommend to do so in order to provide the user the necessary information about what went wrong.

For example getWallet in DSToken.sol

function getWalletAt(uint256 _index) public view returns (address) {
     require(_index > 0 && _index <= walletsCount, "Invalid Index");
     return walletsList[_index];
 }

Messages were added to require statements at commit 3caf2fa7..41f4.

Forwarding Proxy Implementation

The proxy’s implementation is a very simple forwarding proxy. It has some disadvantages like the possibility of storage and selectors collision if used incorrectly.

The team informed us that they have considered multiple proxy contracts and they think their current proxy implementation meets their requirements while maintaining the smallest footprint possible.

We agree with the team decision. We have not found any issue: storage is confined to a single contract per deployed module so any possible collision should be immediate to spot. The proxy contract only implements two public functions, it should be easy to find possible collisions of selectors.

Conclusion

The contracts are well written and they have a proper amount of documentation.

An important change in the audited contracts is the migration from Eternal Storage to simpler storage contracts. This change eliminates the issue we raised in a previous audit.

We have found only a couple of minor issues that do not compromise the security of the contracts. One difficulty is in the Gnosis’ Multisig wallet contract, it is easy to mend and we recommend fixing it before its deployment.

Disclaimer: This audit report is not a security warranty, investment advice, or an approval of the DSToken project since CoinFabrik has not reviewed its platform. Moreover, it does not provide a smart contract code faultlessness guarantee.

Appendix 1: Modules

Note: Solid lines indicate inheritance. Colours are used solely for legibility purposes.

Compliance

This module defines the different policies than can be applied to a security token. This kind of token will select its policy during deployment.

The services that are implemented are the following:

• ComplianceServiceNotRegulated: for tokens without regulation
• ComplianceServiceWhitelisted: for tokens with whitelisted wallets
• ComplianceServiceRegulated: for tokens that are regulated
• ComplianceServiceRegulatedPartitioned: for regulated tokens with partition support
• ComplianceService aggregates the previous compliance services into a single interface.
• The several DataStore contracts are datastores for storing information about the module.
• The two Library contracts provide functions used in most of the module’s contracts.
• ComplianceConfigurationService is an interface to the datastores which allow modifications to the compliance parameters.

Registry

This module contains a register of investors and some properties associated with them like wallets.

• WalletRegistrar allows the owner to register a new wallet.
• RegistryService provides functions for querying and modification of said wallets.

Trust

This module defines different roles and how they interact with a security token.

• TrustService provides the main functionality of the module.
• TrustServiceDataStore is a data-storage which stores trust-related information

Token

A security token implementation based on the ERC20 standard.

• StandardToken is a standard ERC20 Token implementation.
• DSToken and DSTokenPartitioned add more functionality to the standard token.
• TokenLibrary and TokenPartitionsLibrary provide functions and values which are used in several contracts throughout the module.
• TokenDataStore is a storage for token-related data.

Omnibus

Implementation of an omnibus wallet.

Data-Stores

Contracts for data-storage which are used from the other modules.

Service

Allows easy storage and retrieval of the addresses of the different services that compose this project.

Utils

Miscellaneous utilities used by other modules.

This module is a collection of simple, well-known contracts, which are used throughout the project.

• MultiSigWallet is a multi-signature wallet made by Gnosis.
• Migrations allows Truffle’s migrations and it is pulled directly from Truffle’s documentation.
• Ownable allows initialization and transferral of a contract’s ownership.
• Initializable is a common workaround to avoid initializing a contract more than once when using the Proxy pattern.
• Proxy, ProxyTarget and VersionedContract provide the main proxy functionality.