Auditing the Colu Local Network Token

The Colu Local Network (CLN) is a token based on the Ethereum blockchain technology. The Colu team turned to ProtoFire to review and audit their CLN Token smart contracts. ProtoFire has created an actionable report of the issues found while auditing Colu’s smart contracts repository. The report outlines potential attacks, a list of vulnerabilities classified by severity, and additional recommendations to increase security and apply best practices. In this blog post, we publish the issues identified, as well as the updates the Colu team introduced based on the delivered recommendations.

A scope of work

The code to be audited is located in this GitHub repository. The code version used for this report is located at commit [3ac6115].

A list of smart contracts under audit:

• BasicToken.sol: a basic ERC20 token contract implementation. Based on the OpenZeppelin’s StandardToken.
• ColuLocalNetwork.sol: the Colu Local Network contract.
• ColuLocalNetworkSale.sol: the Colu Local Network sale contract.
• ERC20.sol: the ERC20 interface.
• Migrations.sol: truffle migrations.
• MultiSigWallet.sol: a multi-signature wallet, which enables multiple parties to agree on transactions before execution (based on the Gnosis MultiSigWallet).
• MultiSigWalletWithDailyLimit.sol: a multi-signature wallet with a daily limit, which enables an owner to withdraw a daily limit without multisig (based on the Gnosis MultiSigWallet).
• Ownable.sol: a contract for owning a contract with modifiers.
• SafeMath.sol: math operations with safety checks.
• TokenHolder.sol: a token holder contract.
• VestingTrustee.sol: a vesting trustee contract for the Colu Local Network.

Response from the Colu team:

First of all, we want to thank ProtoFire for such professional and comprehensive report. It is reflected that a lot of time and effort was taken in writing this report, and we took each and every recommendation into account and updated the code to reflect that. In the next sections, we will address each issue / recommendation that was mentioned in the report and what or how we changed the contracts in response to it.

The changes introduced can be found in the following commit [528d6b9d].

An overview of the audit and good practices

It’s worth mentioning that the code is well-documented and formatted. The structure of the code is very modular and carefully designed.

Additionally, it encapsulates the SafeMath library, which is used in all math operations. No magic constants were found in the source code.

It is important that all sanity checks, as well as the fail-first approach for precondition, are used across all contracts. As for the for-loops identified in the code, they are all bounded and safe.

The Colu development team wrote 838 tests in total, with most of them covering the functionality.

Attacks against the contracts

We checked the contracts against known attacks, and no vulnerabilities were found.

Vulnerabilities detected in the contracts by severity

1. Issues of critical severity

No critical security issues within the code were found.

2. Issues of high severity

2.1 VestingTrustee#revoke (VestingTrustee.sol)

The transfer result is not checked.

cln.transfer(_holder, transferable);
cln.transfer(msg.sender, refund);

Update by the Colu team:

Although the cln.transfer as it is written can return true or revert the code — thus making the checking of the success of the transfer useless — it is a good practice to check it each time it is used. So, Colu added require() on each cln.transfer in this contract.

2.2 VestingTrustee#unlockVestedTokens (VestingTrustee.sol)

Thetransfer result is not checked.

cln.transfer(msg.sender, transferable);

Update by the Colu team:

Same as in Section 2.1.

2.3 Failing and pending tests

Two pending and five failing tests were encountered. It is recommended to make sure that all the failed tests are fixed before deploying to the Ethereum mainnet.

Update by the Colu team:

Failing test fixed. Pending tests are skipped by default, one can enable them by setting the LONG_TESTS env to true

3. Issues of medium severity

3.1 VestingTrustee.sol

The tokenFallback function and the connected business logic were not covered by tests properly. It is recommended to add the missing tests.

Update by the Colu team:

Tests added to cover the tokenFallback functionality.

3.2 TODO comments (test/helpers/expectRevert.js)

There were two TODO comments left in the code. It is recommended to make sure that all of the outstanding questions are resolved before deploying to the Ethereum mainnet.

Update by the Colu team:

TODOs removed from the code.

3.3 Standard223Receiver#tokenFallback (Standard223Receiver.sol)

Only tkn.sender and tkn.value are used in the source code. It is recommended not to store unnecessary data, as it may result in saving ~20k–80k gas.

tkn = Tkn(msg.sender, _sender, _origin, _value, _data, getSig(_data));

Update by the Colu team:

Removed darta and sig fields from the Tkn struct as it is expensive and not needed. The origin field remains even though it is unused in other contracts.

4. Issues of low severity

4.1 BasicToken#approve (BasicToken.sol)

This code below is preventing the Double Spend Attack, which was acknowledged to be fine. Alternatively, it may be considered to return false instead of revert() since there is nothing to revert yet.

if ((_value != 0) && (allowed[msg.sender][_spender] != 0)) {
revert();
}

Update by the Colu team:

Colu decided to keep revert(), rather than change it to return false, since the EVM doesn’t return a value, so it is hard to distinguish between a succeeded call and a failed one. Moreover, most of the tokens standards use revert\require in this situation.

4.2 Truffle version (package.json)

It is important to make sure that the forked truffle is always up to date while developing.

“truffle”: “git+https://github.com/colucom/truffle.git",

Update by the Colu team:

Colu changed the truffle package to include an unmerge pull request, which addresses the issue of overloading functions in Solidity. The team will keep track with the updates in the truffle package until this pull request (or a similar one) will include in the code, then they will update the code back to the truffle release version.

4.3 Compilation warnings

There were a few compilation warnings. So, it was necessary to check that they are resolved before deploying to the Ethereum mainnet.

Update by the Colu team:

Compilation warning has been fixed.

4.4 Allow an owner to pull out accidentally sent non-CLN ERC20 tokens and ETH (VestingTrustee.sol)

To enable an owner to pull out the non-CLN ERC20 tokens and ETH sent by mistake, the following code can be added:

function withdrawEth() public onlyOwner {
owner.transfer(this.balance);
}
function withdrawERC20(address _erc20Token) public onlyOwner returns (bool success) {
require(_erc20Token != cln);
// do token transfer
}

Update by the Colu team:

It is not possible to send ETH to the contract (no payable function). The withdrawERC20 function was added to allow the owner to withdraw the ERC20 tokens, including the excess CLN tokens not in any vesting plan.

4.5 Install OpenZeppelin via NPM

SafeMath, Ownable, and BasicToken were copied from the OpenZeppelin repository. Moreover, they appear to be copied from the master branch, instead of the release one. This violates the OpenZeppelin’s MIT license, which requires the license and copyright notice to be included if its code is used, and makes it difficult and error-prone to update to a more recent version.

We suggested Colu to follow the recommended way of using OpenZeppelin contracts (via the zeppelin-solidity NPM package). The approach ensures any bug fixes are easily integrated into the code base.

Update by the Colu team:

To be able to change libraries (like SafeMath) in the future and manually concatenate the Solidity files to easily verify them later in etherscan.io, the Colu team decided to leave the code as is. The source reference was mentioned in the code.

Suggestions

1. VestingTrustee#unlockVestedTokens function

It seems that the unlockVestedTokens function is designed to be used by a client directly via a truffle console or any web application. It might be a good idea to add a result code, which could be used via the dry-run call for the tx execution prediction, and/or Error event, which could be captured by an event listener and displayed for a user.

uint constant OK = 1;
uint constant ERR_INVALID_VALUE = 10001;
uint constant ERR_INVALID_VESTED = 10002;
uint constant ERR_INVALID_TRANSFERABLE = 10003;
event Error(address indexed sender, uint error);
function unlockVestedTokens() external returns (uint) {
  Grant storage grant = grants[msg.sender];
// Make sure the grant has tokens available.
  if (grant.value == 0) {
  Error(msg.sender, ERR_INVALID_VALUE);
  return ERR_INVALID_VALUE;
  }
// Get the total amount of vested tokens, acccording to grant.
  uint256 vested = calculateVestedTokens(grant, now);
  if (vested == 0) {
    Error(msg.sender, ERR_INVALID_VESTED);
    return ERR_INVALID_VESTED;
  }
// Make sure the holder doesn’t transfer more than what he already has.
  uint256 transferable = vested.sub(grant.transferred);
  if (transferable == 0) {
    Error(msg.sender, ERR_INVALID_TRANSFERABLE);
    return ERR_INVALID_TRANSFERABLE;
}
// Update transferred and total vesting amount,
  // then transfer remaining vested funds to holder.
grant.transferred = grant.transferred.add(transferable);
  totalVesting = totalVesting.sub(transferable);
  cln.transfer(msg.sender, transferable); // TODO: check a result code
  TokensUnlocked(msg.sender, transferable);
  return OK;
}

Usage:

let trustee = VestingTrustee.at(...);
  let result = await trustee.unlockVestedTokens.call();
  if (result == ERR_INVALID_VALUE) {
    // handle error
    // if tx will fail, do not spend user’s gas with no profit
    alert(“Something goes wrong!”);
    return;
   }
 …
 
  if (result == OK) {
    let tx = await trustee.unlockVestedTokens();
    // make sure that tx does not contain Error event
    let errorEvents = extractEvents(tx, “Error”);
    assertEmpty(errorEvents);
  }

Update by the Colu team:

Added the changes to the VestingTrustee#unlockVestedTokens function as suggested.

2. MultiSigWallet.sol and MultiSigWalletWithDailyLimit.sol

We found out that the MultiSigWallet contracts were copied from the Gnosis repo, so there was a risk of violating the Gnosis licence.

In addition, the contracts were altered. It proves to be dangerous to change well-known and publicly audited contracts. So, we recommended reverting the contracts to the original state.

Update by the Colu team:

The changes introduced to the MultiSigWallet.sol and MultiSigWalletWithDailyLimit.sol contracts were reverted and the source reference was mentioned in the code.

Audit summary

Under the audit, no critical security issues were found. The overall code quality is more than satisfactory. The functions are short, all starting with well-defined and well-ordered preconditions. In general, external effects are left to the end of functions. Most of the code is very well-commented.

Based on the audit recommendations, the Colu team was able to introduce the required changes to comply with the established standards.

Note that this report reflects the current understanding of the known security patterns as they relate to the CLN Token contracts. We have not reviewed the related CLN project. This report should not be construed as an investment advice.