Skip links
Request an Audit
CoinFabrik
Flixxo Token

Flixxo Token Sale Security Audit

Author
Published on:
Published in: Blog

Coinfabrik’s smart contracts auditing team was asked to audit the contracts for the Flixxo Token sale. In the first part, we will give a summary of our discoveries and then the details of our findings.
The audited contracts are in Flixxo repository on https://github.com/AdrianClv/icofunding-flixxo. The audit is based on the commit 739f8dda4fdeffb65a6631ed9d86a42233085cd3, and updated to reflect changes at a2e9ba2cd1953a886c4e367b672cab34f8dede08

By Ismael Bejarano and Pablo Yabo

Summary of the Flixxo Token Sale Security Audit

The audited contracts are:

  • minters/WithdrawTokens.sol: Used to store team and company tokens.
  • minters/WithdrawTokensInput.sol: Used to store the tokens destined as incentives.
  • priceModels/Phased.sol: Implementation of the token pricing model.
  • priceModels/PriceModel.sol: Interface for the token pricing model.
  • token/ERC20.sol: Interface of the ERC-20 Token Standard.
  • token/Expiration.sol: Allows to set an expiration block for some operations.
  • token/Minted.sol: Allows to define which contracts can create tokens.
  • token/MintInterface.sol: Interface for the Minted.sol contract.
  • token/ProjectToken.sol: Flixxo token.
  • token/Token.sol: Standard implementation of the ERC-20 Token.
  • util/Owned.sol: Utility to assign an owner to a contract.
  • util/SafeMath.sol: Arithmetic operations with safety checks.
  • CrowdsaleTokens.sol: Manages the token sale.
  • Scrow.sol: Manages collected funds.

The Flixxo token sale will have a total of 1,000,000,000 (one billion) Flixx tokens issued ever. The price of the tokens will be fixed, but there will be a discount for early investors. The sale doesn’t have a minimum cap and it will always succeed.

The crowdsale will have a hard cap of 300,000,000 issued tokens corresponding to 30% of the total. 10% of the total will be allotted to the development team, another 10% for the company and the rest (50% of the total) will be distributed as an incentive for users and contributors to join the platform.

Detailed findings

Medium severity

Update SafeMath.sol

The version of SafeMath in OpenZeppelin repository has a few modifications that should be applied. A few asserts were removed because they will never be triggered by the EVM.

Current version in Flixxo repository:

  function safeDiv(uint a, uint b) internal constant returns (uint) {
    require(b > 0);
    uint c = a / b;
    assert(a == b * c + a % b);
    return c;
  }

Updated version in OpenZeppelin repository:

  function div(uint256 a, uint256 b) internal constant returns (uint256) {
    // assert(b > 0); // Solidity automatically throws when dividing by 0
    uint256 c = a / b;
    // assert(a == b * c + a % b); // There is no case in which this doesn't hold
    return c;
  }

Fixed in commit e9c5a2eaac56db824a690b122903606ea2113f3f

Extend use SafeMath

  • Function remaining of CrowdsaleTokens.sol should use SafeMath
  // Returns the number of tokens available for sale
  function remaining() public constant returns (uint) {
    return tokenCap - tokensIssued;
  }
  • Functions withdraw and getFee in Scrow.sol should do so too
  function withdraw() public locked {
    // Calculates the amount to send to each address
    uint fee = getFee(this.balance);
    uint amount = this.balance - fee;

    // Sends the ether
    icofunding.transfer(fee);
    project.transfer(amount);

    e_Withdraw(block.number, fee, amount);
  }
  • Function withdraw in WithdrawTokensInput
  function withdraw() public input onlyReceiver {
    uint tokensToIssue = limit((now - startDate) / 24 hours) - numTokensIssued;

    numTokensIssued += tokensToIssue;

    // mint tokens
    if (!MintInterface(tokenContract).mint(receiver, tokensToIssue))
      revert();
  }

Fixed in commit d125913fe463b7cc719378cf6b22ec897167875d and 840b73e7ddd121744d4d4c4d1d2112d8ec273c38

Minor severity

Protection against unintended token transfers

The public contracts CrowdsaleTokens.sol and ProjectToken.sol are not protected against the accidental transfer of tokens.

A user is able to unintentionally send tokens to a contract and if the contract is not prepared to refund them they will get stuck in the contract. The same issue used to happen for Ether too but new Solidity versions added the payable modifier to prevent unintended Ether transfers. However, there’s no such mechanism for token transfers.

We recommend implementing a simple mechanism that allows the contract owner to refund tokens unintentionally sent.

  function refundTokens(address _token, address _refund, uint _value) onlyOwner {
    require(_token != this);
    ERC20 token = ERC20(_token);
    token.transfer(_refund, _value);
    RefundTokens(_token, _refund, _value);
  }

Fixed in commit bd63cd0726beef94216e4c95f1a422ccf3f58970

Double Spend Attack

The function approves in Token.sol is vulnerable to a double spend attack. This attack occurs when a user A approves user B a limit to transfer of X tokens. Then, the same user A wants to approve a Y amount of tokens to user B. If B monitors unmined transactions, it can quickly create a transaction to spend the X tokens previously approved. If B’s transaction is mined first, then B would be able to spend Y tokens even though B just spent X. In this example, A wanted to change the approved value from X to Y but B was able to effectively spend X+Y.

A common mitigation is to require the allowance to be set to zero before changing to a non-zero value.

  function approve(address _spender, uint _value) public returns (bool success) {
    require(allowed[msg.sender][_spender] == 0 || _value == 0);

    allowed[msg.sender][_spender] = _value;

    Approval(msg.sender, _spender, _value);

    return true;
  }

Fixed in commit ce9c04dc6152b7d8d636331bb2b9ff0ab0218440

Require solidity compiler v0.4.15

The reviewed contracts required solidity v0.4.13 but it has two important known bugs, so we recommend updating the pragma directives to require at least v0.4.15.

  • DelegateCallReturnValue “The return value of the low-level .delegatecall() function is taken from a position in memory, where the call data or the return data resides. This value is interpreted as a boolean and put onto the stack. This means if the called function returns at least 32 zero bytes, .delegatecall() returns false even if the call was successful.”
  • ECRecoverMalformedInput “The ecrecover precompile does not properly signal failure for malformed input (especially in the ‘v’ argument) and thus the Solidity function can return data that was previously present in the return area in memory.”

Compliant Transfer event

The function mint in token/ProjectToken.sol issues a Transfer event, but it uses the token address as a source of the Transfer when creating tokens. To be more compliant with the standard and common implementations it should use the null address as a source.

 function mint(address recipient, uint amount) public onlyMinters returns (bool success) {
    totalSupply = safeAdd(totalSupply, amount);
    balances[recipient] = safeAdd(balances[recipient], amount);
    Transfer(0x0, recipient, amount);
    return true;
  }

Fixed in commit 3b8d53fcda3fbc9300afdc32d91af6b9ad6bb9e7.

Enhancements

Token Expiration

The token contract provides an interface to freeze and unfreeze tokens transfer by the contract owner. It is defined in the Expiration.sol contract.

We suggest renaming it to Pausable or Haltable. The name can be confusing since tokens do not expire but are only paused and the contract owner can later unfreeze transfers.

fixed in commit fcbb0ea1f4f29c8be7936b917196bf8a384aefcf

Worrying minting privileges

The mechanism to create new tokens in Minted.sol consists of whitelisting addresses that can create new tokens. This contract whitelisting is done when the contracts are deployed. The deployment script in the repository migrations/2_deploy_contracts.js configures CrowdsaleTokens, WithdrawTokens, and WithdrawTokensInput as minters.

The issue with this whitelist is that it allows creating new tokens long after the token sale is over; some of the contracts will be able to mint for two years after the crowdsale is over. If a bug is discovered within the contracts it could very well end up being exploited by an attacker to create tokens.

This surface of attack can be eliminated by limiting the minting of tokens to the crowdsale contract and instructing the crowdsale to assign them to the WithdrawTokens and WithdrawTokensInput contracts as needed. These contracts will act as a vault of the created tokens, and only transfer the tokens to their beneficiaries when withdraw is called, without requiring any minting privilege.

Move check before formula

In the limit function in WithdrawTokensInput it will be better to make the check before the calculation of formula:

  function limit(uint d) public constant returns (uint) {
    if(d > 3650) {
      return numTokensLimit;
    }

    uint tokensToIssue = ( ( ( (560791145 * d) >> 10 ) - ( d * (d-1) ) * 75 ) >> 1 ) * 10**18;

    return tokensToIssue;
  }

Fixed in commit dd729bc286e8abde46e363337868e63faff11d48

Add invariant check in Phased constructor

The Phased contract relies on its blocks being sorted in ascending order. However, there are no checks that ensure the received array respects this invariant. This could result in unexpected price schemes.

Fixed in commit fad2d3993fb234c47013d373091856a03c88e67a

Call made by calculating function signature inline

In the ProjectToken contract, there is a low-level call issued in approveAndCall:

  function approveAndCall(address _spender, uint256 _value)
    public
    returns (bool success)
  {
    if (super.approve(_spender, _value)) {
      if(!_spender.call(bytes4(bytes32(sha3("receiveApproval(address,uint256,address)"))), msg.sender, _value, this))
        revert();
      return true;
    }
  }

We recommend rewriting this snippet like this:

  interface IGotApproval {
    function receiveApproval(address approver, uint256 value, address token_contract) ;
  }

  function approveAndCall(IGotApproval _spender, uint256 _value)
    public
    returns (bool success)
  {
    if (super.approve(address(_spender), _value)) {
      _spender.receiveApproval(msg.sender, _value, this);
      return true;
    }
    return false;
  }

It is equivalent but explicit, human readable and portable across ABI specification changes.

Incorrect number of minters in the Minted contract

The Minted contract has the following functions:

  function addMinter(address _minter) public onlyOwner onlyIfOpen {
    minters[_minter] = true;
    numMinters++;

    NewMinter(_minter);
  }
  function removeMinter(address _minter) public onlyOwner {
    minters[_minter] = false;
    numMinters--;
  }

These functions lack any checks on the previous state of the address inside the mapping. This allows one to, for e.g, remove a minter when there are none and produce an overflow in numMinters. Since the function is onlyOwner it is not an important issue but call it with care.

Fixed in commit 1a6641c41f486d7ca6505c4f625c1e06510f7c04

Conclusion

We found the contracts to be simple and straightforward and have an adequate amount of documentation.

Some contracts appear to be from a previous version of OpenZeppelin, we suggest trying to use a more recent version where it is appropriate.

The README.md file in the repository is too simple, you probably want to add a few links to the project page and other resources.

We have only discovered medium severity issues, they are simple and easy to fix. We proposed some enhancement which are not required but may be valuable if they are implemented.

The issue found about minting new tokens after the Token Sale wasn’t serious. The contracts take care of minting tokens only up to a predefined amount, and only the receiver is able to create new tokens. It should not affect the general security of the token.

References

Do you want to know what is Coinfabrik Auditing Process?

Check our Smart Contract Audits: The Ultimate Security Guide or you could request a quote for your project.

You may also like