Inbest Token Smart Contract Security Audit

Introduction

Coinfabrik has been hired to audit the contracts for the Inbest Token sale. Firstly, we will provide a summary of our discoveries and secondly, we will show the details of our findings.

Summary

The contracts audited are from the inBest-token repository at https://github.com/inBest-today/inbest-token. The audit is based on the commit 2951db7017f9b53d83c8566454608a495e70f09d.

The audited contracts are:

• InbestDistribution.sol: The preallocation logic for the distribution of tokens.
• InbestToken.sol: The token itself.

The Inbest token sale will have a total of 1 million tokens issued. Half of it will be distributed during the presale period.

No actual security concerns were found. Nevertheless, we make some recommendations to improve code maintainability.  

The following analyses were performed:

• Misuse of the different call methods: call.value(), send() and transfer().
• Integer rounding errors, overflow, underflow and related usage of SafeMath functions.
• Old compiler version pragmas.
• Race conditions such as reentrancy attacks or front running.
• Misuse of block timestamps, assuming anything other than them being strictly increasing.
• Contract softlocking attacks (DoS).
• Potential gas cost of functions being over the gas limit.
• Missing function qualifiers and their misuse.
• Fallback functions with higher gas cost than a what a transfer or send call allows.
• Fraudulent or erroneous code.
• Code and contract interaction complexity.
• Wrong or missing error handling.
• Overuse of transfers in a single transaction instead of using withdrawal patterns.
• Insufficient analysis of the function input requirements.

Detailed findings

Enhancements

Use of outdated solidity compiler pragmas

From version 0.4.21 on it is encouraged to use emit while using events because it improves code legibility and helps differentiate between an event emission and a function call. This is not done in any of the event emission statements.  We recommend developers to use newer features like defining constructors using constructor(…){…} instead of naming them, or explicitly emitting events.

Use of modifiers instead of repeated requires

Along InbestDistribution.sol, the following statement is repeated: 

 require(now >= startTime);

We recommend placing this in a modifier so as to avoid code duplication and to make code more readable. This way, the code is less cluttered, facilitating possible future changes.

In the same vein, we propose taking away the following statement at the transferTokens function:   

require(now >= allocations[_recipient].endCliff); // Cliff period must be ended

Since this check is already done inside the call to SafeMath:

now.sub(allocations[_recipient].endCliff)

This is done at the setting of newAmountClaimed inside the if. Since the “else” branch is followed when now is greater than or equal to the time marking the end of the vesting, the check ends up being redundant, as the end of the vesting is always greater than the cliff end.

Use of variables in transferTokens

The development team would probably like to refactor the following line:

newAmountClaimed = allocations[_recipient].totalAllocated.mul(now.sub(allocations[_recipient].endCliff)).div(allocations[_recipient].endVesting.sub(allocations[_recipient].endCliff));

It is advisable to use local variables to give more meaning to the code, considering that it improves maintainability and legibility.

Use of AllocationType type

In the definition of Allocation structs, the allocationType member is specified as an uint8. It would be more legible if the Allocation struct dealt with AllocationTypes directly. This would mean that it wouldn’t be necessary to cast the types to construct Allocations thus improving maintainability. This would improve maintainability.

Use of construction parameters

The development team could have the token supply passed through a construction parameter. By doing this,  it would be defined through a deployment script. That would make the following check superfluous:

require(AVAILABLE_TOTAL_SUPPLY == IBST.totalSupply()); //To verify that totalSupply is correct

Similarly, this check could be done in a deployment script:

require(AVAILABLE_TOTAL_SUPPLY == AVAILABLE_PRESALE_SUPPLY.add(AVAILABLE_COMPANY_SUPPLY));

This would mean that the token supply would have to be passed to the token constructor as a parameter. Another advised change would be trying to define DECIMALFACTOR according to IBST.decimals(). This would ensure that any future changes to the contract would propagate to the InbestDistribution.sol contract, reducing the probability of introducing an error.

Observations

We write here some observations we made when reviewing the contracts.

• None of the contracts used call(), send(), nor transfer() builtins.
• The mathematical operations performed in the contracts are protected against overflow using requires for input parameters.
• There are only four possible calls to another contract in InbestDistribution, besides the one in the refundTokens function:
  • When checking that the total supply in the token contract is the same as the available total supply, calling a getter.
  • When transferring tokens, both when claiming the allocation for an address and when transferring from the company allocation. This is done using the token’s transfer function implemented by StandardToken.sol by OpenZeppelin making it is safe from reentrancy attacks.
  • When refundTokens is called. More on that below.
• Timestamps are used to verify the start time for the sale is in the future. While it is possible for allocations to be locked if a malicious miner sets the timestamp as greater than the starting time before it is necessary, such an attack is not probable due to the fact that a desynchronized node can’t build consensus. This could be completely mitigated by setting the start time a few hours later than the deployment of the contract and setting the allocations right away.
• No function in the contracts have a loop that could be abused to cause a soft lock or an unbounded usage of gas.
• InbestDistribution has a refundTokens function
  • We commend the devs to provide a mechanism for refunding tokens mistakenly sent to the contract.
  • Since it is modified by onlyOwner, calls to other contracts are controlled.

Conclusion

We found the contracts to be simple and straightforward and to have an adequate amount of documentation. No urgent security concerns were found with the code, but we advice implementing the changes proposed in this document. We found the contracts to use well tested libraries and the devs made to make sure that input preconditions were explicit. There were some possible issues in the style of the contract but none was urgent.