Coinfabrik smart contract security auditing team was asked to audit the contracts for the CDX platform. In the first part, we will give a summary of our discoveries and then the details of our findings.

The contracts audited are from CDX’s repository at https://github.com/commodityadnetwork/CDX. The audit is based on the commit c05cf82983904b15a4b0ec790cce9e583d96bc0f.

Summary

The audited files are:

• Owned.sol: a common contract for function access control.
• ERC20.sol: The ERC20 Token interface.
• Math.sol: checked math functions.
• Roles.sol: has three contracts for a more fine-grained function access control. It allows to multiple addresses to have call permits on different functions, defined by their role.
• ContributionRegistration.sol: a simple mapping of dates and indexes to IPFS hashes.
• TokenPool.sol: independent contract for handling a pool of tokens
• TokenIceBox.sol: a separate contract that handles the token pool balance.

Detailed findings

High severity

No high severity issues have been found.

Medium severity

Documentation could be better

Functions and contracts do not specify their intent. The same happens to many inlined comments not specifying the purpose behind some of the code lines. This makes it harder to understand since it is difficult to infer the intent of a function or a contract without an appropriate documentation describing what the code does.

Safe multiplication fails for some values

More importantly, the require statement in the multiplication function isn’t exhaustive; it may overflow for some operands and not revert when it should:

 function mul(uint256 x, uint256 y) constant internal returns (uint256 z) {

 z = x * y;

 require(z == 0 || z >= (x > y ? x : y));

 }

For example, it will overflow with x = (3 << 127) and y = (3 << 127), and will result in z = (1 << 254), which passes the requirement.

 function mul(uint256 x, uint256 y) constant internal returns (uint256 z) {

 z = x * y;

 require(x == 0 || (z / x) == y);

 }

Missing visibility in functions

Visibility defaults to public on functions without specified visibility. Consider adding public or internal qualifiers to functions which should be accessed from outside or inside the contract respectively.

ContributionRegistration index may be too small

ContributionRegistration storage can be modified by oracles. For each day, there are 256 entries available, if this limit is reached some functions will stop working, consuming all gas for that particular day since they get stuck in an infinite loop.

 function getLastHash(bytes10 date) constant returns (string hash) {

     uint8 index = 0;

     while(keccak256(contributionLists[keccak256(date, index)]) != keccak256("")) {

         hash = contributionLists[keccak256(date, index++)];
     }
 }

 /* the date is in the format dd-mm-yyyy and we add 1 to it until no more entries are found */

 function addContributionList(bytes10 date, string ipfsHash) roleOrOwner("oracle") public returns (bytes32 index) {

     uint8 i = 0;

     index = keccak256(date, i);

     while(keccak256(contributionLists[index]) != keccak256("")) {

         index = keccak256(date, ++i);
     }

     contributionLists[index] = ipfsHash;
 }

Also, having functions with unbounded gas usage can lead to a denial of service attack to the contract.

We suggest in another issue to replace the mapping to a bytes32 with a mapping to an array. This fixes both problems: access to an array use constant gas and allows above 256 entries.

Minor severity

Varying and outdated solidity version requirements

Some contracts enforce Solidity version 0.4.15 while others enforce Solidity version 0.4.13. Consider upgrading the requirements to the last version available.

Duplicated functionality in Auth and Owned classes

Auth has an additional feature over Owned which is a callback to the designated authority. This callback is unused in the project so all instances of Auth can be replaced with Owned. Consider doing it if you are not going to use this functionality in the future.

Enhancements

Math.sol has unnecessary functionality for the project

Most of the functions are not used, consider using a smaller implementation like OpenZeppellin’s SafeMath.

Unnecessary hashing

The ContributionRegistration contract uses index rehashing as a way to create multiple entries for a given key in mapping:

 contract ContributionRegistration is SecuredWithRoles {

 	 mapping(bytes32 => string) contributionLists;

	 // TODO add campaigns too in order for the providers to be able reconstruct the contribution pool

	 function ContributionRegistration(address roles) SecuredWithRoles("ContributionRegistration", roles) {

	 }

	 function hasHash(bytes10 date, uint8 idx) constant returns (bool) {

    	 return keccak256(contributionLists[keccak256(date, idx)]) != keccak256("");

	 }

	 function getHash(bytes10 date, uint8 idx) constant returns (string) {

    	 return contributionLists[keccak256(date, idx)];

	 }

	 function getLastHash(bytes10 date) constant returns (string hash) {

    	 uint8 index = 0;

    	 while(keccak256(contributionLists[keccak256(date, index)]) != keccak256("")) {

        	 hash = contributionLists[keccak256(date, index++)];
    	 }
	 }

	 /* the date is in the format dd-mm-yyyy and we add 1 to it until no more entries are found */

	 function addContributionList(bytes10 date, string ipfsHash) roleOrOwner("oracle") public returns (bytes32 index) {

    	 uint8 i = 0;

    	 index = keccak256(date, i);

    	 while(keccak256(contributionLists[index]) != keccak256("")) {

        	 index = keccak256(date, ++i);
    	 }

    	 contributionLists[index] = ipfsHash;
	 }
 }

This can be simplified by creating a dynamic array from the value of the mapping:

 contract ContributionRegistration is SecuredWithRoles {

 	 mapping(bytes10 => string[]) contributionLists;

 	 function ContributionRegistration(address roles) SecuredWithRoles("ContributionRegistration", roles) {}


 	 function hasHash(bytes10 date, uint8 idx) constant returns (bool) {

     		 return idx < contributionLists2024.length;
 	 }

 	 function getHash(bytes10 date, uint8 idx) constant returns (string) {

     		 return contributionLists2024[idx];
 	 }

	 function getLastHash(bytes10 date) constant returns (string) {

    	 if (contributionLists2024.length == 0)
        		 return "";

    		 return contributionLists2024[contributionLists2024.length - 1];

	 }

 	 function addContributionList(bytes10 date, string ipfsHash) roleOrOwner("oracle") public {

     		 contributionLists2024.push(ipfsHash);
 	 }
  }

Unused base contracts for TokenPool

TokenPool inherits from ERC20Events and Math, but it doesn’t use the events declared within that contract or the safe math functions. We recommend removing unnecessary dependencies that may cause problems:

contract TokenPool is SecuredWithRoles {

Notes

• Because these contracts are upgradeable, they can be changed by the owners during the crowd sale, altering the initial behavior. Even though it may be useful if an issue is found, it may potentially change the business rules.

Conclusion

Even though no critical vulnerabilities have been found, the documentation was poorly written and we had difficulties finding the purposes of the contracts. Having said that, overall code quality was correctly written, as it shows there is a solid grasp on the way contracts work.

References

• CDX Token Repository: https://github.com/commodityadnetwork/CDX
• OpenZeppelin SafeMath: https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/math/SafeMath.sol
• ERC-20 Token Standard: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20-token-standard.md