Investx Token Sale Review

Reviewing Investx ERC-20 Smart Contracts

We have been asked to perform an audit of the ERC-20 contracts which will be used for Investx token sale. This audit represents an internal audit which we have performed which should be part of wider due-diligence undertaken.

The contract audit is broken down into several sub sections. We have also been given access to the requirements of the project and have confirmed logic & business requirements against our understanding of the codebase.

Report sections below

• Code Validation & Security Checks performed
• Recommendations & Observations
• General Observations
• Actions/Amendments made from initial report
• Consulsion

The Audit

Audit is based on the following contracts held under within repository https://github.com/investxltd/inx-token-sale, the final commit hash looked at is f5ea79a795e1c2dcf56db020a6025c4c8d6e2525 branch master

• INXCrowdsale.sol — responsible of crowdsale logic
• INXToken.sol — the ERC20 contract
• WhitelistedMintableToken.sol — a specialised mint-able whitelisting contracting
• MintedKYCCrowdsale.sol — a specialised KYC whitelisting contract

Final contracts are based on OpenZeppelin contracts, version 1.12.0 and solidity compiler version 0.4.24.

Code Validation & Security Checks Performed

This is a comprehensive but not exhaustive checklist especially since there may be unknown bugs within the EVM, Solidity exploits and uncaught errors. Below is a list of checks performed on the contracts as mentioned above.

• Misuse of the different call methods: call.value(), send() and transfer().
• Race Conditions (Reentrancy, Cross-function Race Conditions)
• Transaction-Ordering Dependence / Front Running
• Timestamp Dependence
• Integer rounding errors, overflow, underflow and related usage of SafeMath functions
• Denial of Service with Unexpected Revert
• Denial of Service with Block Gas Limit
• Denial of Service with Soft Locking attacks
• Forcibly Sending Ether to a Contract
• Compiler and base contracts version checks
• Missing function qualifiers and their misuse
• Code and contract interaction complexity
• Overuse of transfers in a single transaction instead of using withdrawal patterns
• Insufficient analysis of the function input requirements

Recommendations & Observations

Below I have listed out a set of observations and recommendations which I have broken down into a three categories.

• Critical — a bug seen as a critical issue which needs addressing immediately
• High — a missing feature which has been highlighted as a requirement
• Medium — something which we believe would be beneficial but may not be a requirement
• Low — a nice to have or an opinion on style/direction of the code

Critical

• No critical security issues could be identified.

High

• INXCrowdsale — openingTime & closingTime is set to an arbitrary small number, this looks to be for testing only but will need to be modified before go live. (resolved) ✅
• INXToken — The property founderTokensLockedUntil is set to an arbitrary small number, this looks to be for testing only but will need to be modified before go live. The property is not changeable once deployed and the property is defined on contract creation. (resolved) ✅

Medium

• Upgrade OpenZeppelin contracts to 1.12.0 (resolved)
• Upgrade contracts to use fixed Solidity version 0.4.24 (resolved) ✅
• INXCrowdsale — _preValidatePurchase() checks beneficiary is whitelisted but not the sender (resolved) ✅
• INXCrowdsale — missing validation on preSaleRate, set on contract construction (resolved) ✅
• INXCrowdsale — no required checks for preSaleRate on construction. (resolved) ✅
• INXCrowdsale — no validation on preSale rate to enforce its less than the normal rate. (expected)
• INXCrowdsale — missing validation on addManyToKyc(), removeFromKyc(), addToInxWhitelist(), removeFromInxWhitelist() (resolved) ✅
• INXToken — transfer() and transferFrom() checks sender is whitelisted but not the beneficiary. (expected)
• INXToken — missing validation on setInvestxPlatform() (resolved) ✅
• INXToken — missing validation on addAddressToFounders() (resolved) ✅
• INXToken — there is no way to rollback enableTransfers() i.e. disableTransfers() (expected)

Low

• INXToken — potential missing events — enableTransfers(), addAddressToFounders(), setInvestxPlatform()
• INXCrowdsale — potential missing events — setRate(), setPreSaleRate(), setClosingTime(), setMinContribution(), publicSale()
• INXToken & INXCrowdsale can receive accidental Ether, e.g. via suicide or self destruct, but there is no way of extracting this — recommendation is to add OpenZeppelin HasNoEther which will allow extract of wrongly received Ether. (resolved) ✅
• INXCrowdsale — the method publicSale() mutates state, change to be more descriptive e.g. triggerPublicSale() or moveToPublicSale()
• INXToken — Minor discrepancy noticed in method addAddressToFounders() which uses external and not public.
• Splitting out contracts from the internal dashboard project (resolved) ✅

General Observations

• INXCrowdsale — Timestamps were fixed at contract creation, and later the end date is able to be changes by the owner. I see minimal possibility of misuse to take advantage of the token sale other than Investx be able to extend the crowdsale to a later date.
• INXCrowdsale — no soft or hard caps defined — the only way to stop token purchases is by calling pause() or removing people from the whitelist. This is by design due to the proposed mint and burn processes as part of the tokenism if design.
• INXToken & INXCrowdsale — Whitelist functions correctly have management whitelist functionality.
• INXToken — currently any owner of the tokens can burn the tokens they own.
• INXCrowdsale — Ownership is split with two different levels of control. Correctly owner has ultimate control still with the ability to safely transferring ownership which is possible only by the owner themselves.
• All contracts do not have a upgrade path which is common for the purposes of the crowdsale.

Actions/Amendments

Changes to the above points have be made and I have re-reviewed the smart contract code. I have update the above with resolved or expected if it has been addressed or are by design.

• Resolved meaning that the issue was raised and retrospectively addressed.
• Expected meaning this is by design, raised and discussed this is left as is.

Branch master commit f5ea79a795e1c2dcf56db020a6025c4c8d6e2525

Static analysis

We also run the contracts through some static analysis tools which came out with no obvious issue, tools used can be seen below:

• https://github.com/ConsenSys/mythril
• https://securify.ch — web based analysis tool
• https://tool.smartdec.net/ — web based analysis tool

Conclusion

All in all I thought the contracts where well written and simple to understand. I did not find any known critical security issues.

The contracts extend core functionality from OpenZeppellin which is our advised starting point for a new token sale. Small amounts of additional logic have been added, the most significant being that new tokens are minted on purchase. Functionally this happens as the crowdsale calls back to the token contract to assign and mint new tokens to the beneficiary.

We identified some traits which have highlighted above to be addressed. Most of these things relate to missing validation, missing events and minor corrections which can be seen in the recommendations above.

All of the above points highlighted have been ratified and code coverage is high throughout the code base.