The Hubii team asked us to review and audit their crowdsale and “Hubiits” (HUB) token contracts. We looked at the code and now publish our results.
The audited contracts can be found in the CoinFabrik/ico repository. The version used for this report is commit
We’ve found the code to be very modular and well documented. Here’s our assessment and recommendations, in order of importance.
Update: Hubii team followed most of our recommendations in the latest version of the code.
State transitions are implicit and not enforced
There are multiple states a
Crowdsale instance can be in. The current state can be obtained by calling
getState, and functions are enabled or disabled according to it. Said function also gives an idea of the desired sequence of states that the crowdsale should follow. It’s easy to see, however, that there are a lot more possible state transitions than those implied:
- If at any point in time the finalizeAgent variable is changed into a non-sane instance, a crowdsale can return to the
- Once the crowdsale has been considered successful (being in the
Successstate), it’s possible to resume the
Fundingstate by setting a new cap.
These kind of unexpected state transitions can have unexpected consequences, create security holes, and complicate maintenance and further development. Fix this by making transitions explicit and enforcing the states in which each function can be called.
Update: Hubii replied “The function where the state transitions were possible were marked as
onlyOwner, lowering the possibility of outside attack. Despite the function having the
onlyOwner modifier, we modified the contracts to prevent unexpected state transitions and removed the
Preparing state.” Fixed in
Incorrect investor count
preallocate in Crowdsale.sol do not increase the
investorCount variable. If the intention is to count investors who transfer funds using this function, consider increasing the variable in this method.
Update: Hubii replied that the meaning of this variable was by design. Hubii replied “The
investorCount was designed to count investors in the crowdsale, and not early investors assigned through the preallocate function. It is designed in this way.”
Furthermore, the check in line 191 for
investedAmountOf will not increase the investor count for regular purchases if the calculated price is zero. Consider changing the guard to check for
Update: Hubii replied “Regular purchases are required to send non-zero amounts and the
investorCount is only increased when
investedAmountOf is zero to avoid counting the same address twice. Early investors who also purchased tokens through the crowdsale will also be counted correctly by checking
investedAmountOf, which won’t happen if we check
Inconsistent order of arguments of calculatePrice
calculatePrice is defined in
PricingStrategy to take five arguments including
tokensSold. These two are swapped in the implementation of the function in
FlatPricing and its use in
Crowdsale. It presently causes no problem because the implementation and usage are consistent. If a new pricing strategy is implemented using the documented order, the contract can break. Fix the interface to use the same order:
Update: Hubii replied “The name of the arguments and the documentation was fixed to reflect the actual usage. But, this issue is not severe since it does not affect the contract behavior.” Fixed in
Token contract creator has control over token emission and release
Instead of the
Crowdsale acting as owner of the token, as in most implementations (see OpenZeppelin’s
Crowdsale, there is an account that retains ownership of the token. This allows the owners to appoint themselves as minting agents at any point during the crowdsale and emit new tokens, or release the token before the crowdsale ends. This reduces the public trust on the crowdsale mechanics. Move ownership of the token to the
Crowdsale contract, and maintain control only over the upgrade mechanism by appointing the team as
Update: Hubii replied “As suggested in another issue the setup was changed to create all the contracts from a single contract. In this way, the
Crowdsale contract is the owner of the
CrowdsaleToken.” Fixed in
Strategies and limits can be modified mid-crowdsale
setFundingCap in Crowdsale.sol allow the owner to change the crowdsale rules while the crowdsale is running, changing when the crowdsale ends, the funds cap, the price and the maximum investment per address, at any stage. This requires complete trust on the owner of the crowdsale. Consider requiring these parameters at construction time, or restricting these changes to the
Preparing stage of the crowdsale only.
Update: Hubii replied “These methods are declared internal to allow modifications only during the deployment of the
Crowdsale contract, and thus, the subcontracts cannot be changed afterwards.” Fixed in
Preallocate function has no sanity checks
preallocate in Crowdsale.sol L239 has no checks regarding the amount of tokens being non-zero, the receiver address being non-zero, the state of the
PreFunding (allowing preallocations to be sent even on a
Finalized crowdsale), or the cap not being exceeded. Even if the method is to be invoked only by the
Crowdsale owner, consider adding checks for all these conditions to prevent inconsistent states produced by human errors.
Update: Hubii replied “Checks were added to ensure correct values are processed by the
preallocate function to avoid any problems.” Fixed in
Contracts are unnecessarily generic and complex
- The crowdsale contract can optionally have a
minimumFundingGoal, which can trigger a
Failurestate if set.
- The token has a
mintableflag that controls whether it can actually mint, even though it extends
The many different flags and available behaviors interact in many ways, resulting in a very complex contract. More complexity means a larger attack surface. Rather than conditionally controlling different behaviors via flags, consider settling on a single behavior per class to reduce complexity.
Update: Hubii replied “Some complexity is necessary if your contract has to provide several options. We think that altering significantly the contracts is too risky at this stage of development.”
Contract initialization is too complex
The excessive use of the strategy pattern for the crowdsale contract makes deployment and configuration much more complicated, requiring several agent configurations (
setFinalizeAgent), which is highly error prone. Consider defining a specialized version of
Crowdsale (maybe calling it
HubiiCrowdsale) that performs all required initialization steps, including setting up and configuring the agents and token contracts.
As a side note, note that all
setAgent methods can be invoked at any time, not just during construction. This was one of the issues that enabled the Parity MultiSig hack, though in this case the
onlyOwner modifier prevents similar attacks.
Update: Hubii replied “The split of the contract in several pieces was done to circumvent the gas limit in the mainnet last year. We understand this is not an issue now, and we have grouped the setup process within a single contract.” Fixed in
Owner can load more refund money than necessary
loadRefund allows to send balance into the contract so that it can be used for refunds. This money can only be withdrawn via the
refund function, by an investor requesting a refund of the same amount they had invested.
It’s possible to load more balance than necessary, and the contract provides no mechanism to withdraw the extra balance. (Note that it would be possible to withdraw it by abusing the lack of safety checks in
preallocate, as explained below, but this should not be relied upon.) Consider requiring that
msg.value == weiRaised in
Update: Hubii replied “We decided to assign the excess of the refund ether to the sender address. So they can refund the excess through the normal mechanism.” Fixed in
Unclear whether a finalize agent is required for a crowdsale
Although the code of the finalize function points out that “Finalizing is optional. We only call it if we are given a finalizing agent”, the
Crowdsale can never move out of the
Preparing state as per L481 if no finalize agent is set.
If the intention is for the finalize agent to be mandatory, consider accepting it as part of the constructor parameters, and remove the method to change it on the fly. On the other hand, if the intention is to support no finalize agent, ensure there is an alternative way to release the tokens, which is currently responsibility of the finalize agent.
Update: Hubii replied “As part of another issue the
Preparing stage was removed, and now the
finalizeAgent is mandatory.”
Fund receiving address can be changed by owner for a brief period
setMultisig function allows the owner to change the address where funds are sent. It can be used for a brief period after the crowdsale is deployed, before the investor count reaches 5. The purpose stated in the comments is to be able to fix it before the crowdsale has begun, but this is not enforced by the contract.
If the crowdsale is already running, with less than 5 investors, the owner can still change the address. If by error or malice he sets an invalid one, soon afterwards it will be impossible to change, once investor count reaches 5. For this reason, it’s not a good safety measure as it stands. Consider removing the function, or adding the
Update: Hubii replied “The function to change the receiving address was declared
internal to only allow modifications when the contract is being deployed.” Fixed in
Ownable’s transferOwnership fails silently
transferOwnership there’s a sanity check to see that the address argument is non-zero, which can fail silently. Consider throwing if the check doesn’t pass, by changing the line to
require (newOwner != 0x0);.
Update: Hubii replied “The
transferOwnership function was modified to require the
newOwner to be non null.” Fixed in
Use SafeMath in all math operations
Most math operations are safe, but there’s still a few that are unchecked (these two, for instance). It’s always better to be safe and perform checked operations.
Update: Hubii replied “We updated to use safe math operations where they were not used.”
Strategy and agent contracts can be set to invalid ones
CeilingStrategy as well as
FinalizationAgent each have an
isSane function and an accompanying one in
Crowdsale, whose apparent purpose is to validate their correct setup with respect to a
Crowdsale instance. Although the instances can be changed mid-crowdsale, they are not validated, and the contract could be left in an invalid state. Consider adding the validation for each in the setters
setFinalizeAgent. For example, add
isPricingSane(); after line 396.
Update: Hubii replied “Now setters are internal to prevent modifications during the crowdsale, and we now check if they are valid.” Fixed in
Current code specifies version pragma
^0.4.11. We recommend changing the solidity version pragma to the latest version (
pragma solidity ^0.4.14) to enforce the use of an up to date compiler; especially considering the latest version includes a security fix for the
ecrecover function, which Crowdsale.sol uses.
Update: Hubii replied “We updated our requirements to solidity v0.4.13, and removed the use of
ecrecover since it was not essential to the
Crowdsale. We didn’t update to v0.4.14 because it isn’t supported on Truffle yet.”
The compiler issues the warning Variable is declared as a storage pointer. Use an explicit “storage” keyword to silence this warning in MultiSigWallet.sol. Consider adding the required keyword to remove this warning.
Update: Hubii replied “We have added the keyword storage as suggested.” Fixed in
ERC20 decimals field should be uint8
Update: Hubii replied “We have updated the contracts to reflect the standard and declared decimals as
uint8.” Fixed in
Undocumented interface of UpgradeAgent
Compared to the rest of the code,
UpgradeAgent is lacking in documentation and clarity. Since this is intended to be used by future developers, we believe it’s specially important that it be very well documented.
Our understanding is that a future upgraded token contract should implement an
upgradeFrom function that receives a token owner and an amount of tokens (within the owner’s balance) as parameters to transfer from the older to the new token. This function is called by the older token.
Consider renaming the parameters to owner and amount, which are more representative of their meaning. Also, consider enforcing in
upgradeFrom be only callable from the old token.
Update: Hubii replied “We have added documentation to the
UpgradeAgent to help understand its functionality.” Fixed in
Low test coverage
There is a single test file in the codebase, which only checks a few methods on the
Crowdsale contract. Consider adding tests for finalization, refunds, crowdsale failures, preallocation, investment with customer ID and signature, among others; as well as tests for other contracts, such as the
CrowdsaleToken, including tests for the releasing and upgrading mechanisms.
Also, all tests are executed on the same set of deployed instances. This restricts all tests to be run in succession and on a single configuration, limiting testing to a single set of constructor parameters for all contracts. Consider setting up different instances on different test suites to accommodate for different configurations, as well as adding unit tests for all contracts using mocks where needed.
Update: Hubii replied “We have added several tests since the audit, and we are adding more.”
Does not install libraries via a package manager
Though the code uses OpenZeppelin’s
SafeMath, code is copied and pasted into the repository, including some style modifications such as renaming
uint. This makes it difficult to include any security changes included with new releases of the library, as they must be manually integrated. Consider using npm for installing OpenZeppelin.
Update: Hubii replied “We will consider this for future changes in the repository.”
Token for Crowdsale must be Mintable
Crowdsale.sol states in L36 that the token contract must be
FractionalERC20 (even though this is not enforced at runtime). However, in
assignTokens in L497, which is invoked from both
invest methods, the token contract is forced to
MintableToken. Consider changing the token field type to
Also, if the
CrowdsaleToken contract is used as a token for the
Crowdsale, the token’s flag
mintable in the constructor must be set to true for the crowdsale to work. Consider removing this flag altogether, and always making
mintable == true.
Update: Hubii replied “The
Crowdsale contract was changed to always expect a
Crowdsale token. We require the
CrowdsaleToken to be mintable in this instance of the
Refunding is not trustless
The crowdsale has logic to return the funds to original investors if the minimum cap is not reached (seeL437). However, this depends on the team from manually transferring the funds back to the
Crowdsale via the
loadRefund method, requiring the public to trust in a third party for effectively issuing the reimbursements. Consider locking the funds in a contract specifically designed for it until the crowdsale is known to have failed or succeeded, and only in the latter case transfer them to the team.
Update: Hubii replied “The audit is correct to point that the refunding in case of the crowdsale not reaching the minimum cap is not trustless. We consider that fixing this issue at this point of the schedule will add more risks than solutions. But we will consider it for future reference.”
There’s a problem with using timestamps and
now (alias for
block.timestamp) for contract logic, based on the fact that miners can perform some manipulation. In general, it’s better not to rely on timestamps for contract logic. The solutions is to use block.number instead, and approximate dates with expected block heights and time periods with expected block amounts.
The Crowdsale.sol contract uses timestamps in lines 484–485. The risk of miner manipulation, though, is really low. The potential damage is also limited: miners could only slightly manipulate when the crowdsale starts or ends. We recommend the team to consider the potential risk of this manipulation and switch to
block.number if necessary.
For more info on this topic, see this stack exchange question.
Update: Hubii replied “The contract has been updated to use block number instead of timestamp.” Fixed in
Notes and Additional Information
- Good job fixing warnings and typos from Consensys MultiSig wallet! Consider sending a pull request to the official repository so these changes are integrated back into main development.
- Consider making
- In order to improve clarity, consider moving precondition checks in methods to modifiers in all methods in BonusFinalizeAgent.sol and UpgradeableToken.sol, as well as in investment methods,
- Consider reusing the
setCeilingStrategyfunction in place of the two equivalent lines in the
Crowdsalewill accept dates in the past as parameters for start and end. Consider adding sanity checks for that.
- Prefer expressing rate as
weiPerTokenin FlatPricing.sol, so the amount of tokens to be issued per investment can be calculated as a multiplication instead of a division, avoiding rounding errors.
- Consider including the unit in which the bonus is expressed as part of the variable name to avoid confusions in
isSanein line 21 of PricingStrategy.sol could be made abstract. This would make it harder for a developer implementing a child contract to forget implementing the method.
- Comment in line 54 of MintableToken.sol is misleading. “Only crowdsale contracts are allowed to mint new tokens.” Any address can be a mint agent.
Haltablebut this is only used by adding the modifier
finalize. Consider adding that modifier to other independent state-modifying functions such as
- The comment in line 80 of UpgradeableToken.sol is incomplete.
- Consider renaming
ReleasableTokento something more descriptive, such as
- Comment in line 47 of Crowdsale.sol seems to be wrong: reads “tokens will be transfered from this address” but should be “ether will be transferred to this address”
- Add check for
minimumFundingGoalin Crowdsale.sol constructor to be under the ceiling cap.
- Consider extracting the
Preallocateablecontracts respectively, to improve modularity.
- Consider breaking BonusFinalizeAgent.sol into two contracts: a base one with the
isSanecheck (if needed) and the
releaseTokenTransfercall upon finalization, and a derived one with the bonus logic itself.
- The check for
finalizein L317 of Crowdsale.sol is redundant, since the method can only be called in state Success as per its modifier.
investInternalin line 171 of Crowdsale.sol and
preallocatein line 239 of Crowdsale.sol share a lot of code and could be refactored to using a common function.
getStateis called twice in the same function in
investInternal. Consider saving the state to a variable to avoid the double call. This will also save gas.
- Consider changing all
assert(send)calls in Crowdsale.sol (L207, L212 and L443) to
- Consider extracting the magic constant
10000from L56 of
BonusFinalizeAgentinto a constant state variable, for clarity and maintainability.
We’ve found the code to be very modular and well documented. Six severe security issues were found and reported, along with how to fix them. Some changes were proposed to follow best practices and reduce potential attack surface.
Update: Hubii team clarified that some of the issues found were design decisions rather than vulnerabilities. The team fixed most of the issues following our recommendations.
Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to the HUB token and crowdsale contracts. We have not reviewed the related Hubii project. The above should not be construed as investment advice or an offering of HUB tokens. For general information about smart contract security, check out our thoughts here.