Skip to content

Instantly share code, notes, and snippets.

@holiman

holiman/Bancor-CrowdsaleAudit.md Secret

Last active May 26, 2017 11:13
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save holiman/a5e8690cd1dcc875b9f4f39297d587be to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save holiman/a5e8690cd1dcc875b9f4f39297d587be to your computer and use it in GitHub Desktop.
Download ZIP
Raw
Bancor-CrowdsaleAudit.md

Background and summary

These document is an audit report about the Bancor protocol crowdsale contracts, which have been audited by Martin Holst Swende on behalf of Dyno Security AB. During the development process, there have been three preliminary reports with potential issues, which have been addressed during the course of development.

In general, the Crowdsale contract has a strong security model by design, whereby many types of typical errors can be avoided or addressed post-facto. The design feature allowing this is that the contributed funds (ether and tokens) are not collected within the Crowdsale contract, but is instead passed to a beneficiary.

This means that assets, during and after the crowdsale, is in the control of the beneficiary once the asset has been transferred (with the exception of ether residing within the EtherToken, but those can be collected by the beneficiary at will).

  • if a user were to make an erroneous transfer of a token directly to the beneficiary or crowdsale, it can be sent back without loss of funds.
  • If a user erroneously transfers 'native' tokens to a contract which does not support ERC20 tokens, the error can be corrected without loss of funds, via destroy and issue
  • if a vulnerability is found within the suite of crowdsale contracts, there is no 'bounty' to withdraw ether or tokens from (aside from the actual native crowdsale tokens which could be rescued by simply bootstrapping a new token with the balances prior to the hack).

The corollary of having this security model, however, means that

  • The funds raised within the crowdsale are not tied to any minimum-success thresholds; all funds are immediately at the disposal of the beneficiary
  • The trust model of the crowdsale places total trust in the beneficiary and tokenOwner, who - at will - can use all funds and/or issue/destroy tokens.

The security of the beneficiary account is not within scope of this audit; it's assumed to be a highly secure multisig wallet.

Regarding the allocation of funds/tokens, at any time during the crowdsale,

  • beneficiary owns 50% of all tokens
  • beneficiary owns all underlying assets

Based on the audit, we estimate the risk level of the Bancor crowdsale contract(s) to be Low.

Scope

This report covers the contracts central to the Bancor crowdsale, and is part of a larger audit covering all Bancor contracts. This report was performed on commit hash 038e221882c7cf745f2c31fa839cee4a4397b5e5 of the contracts on https://github.com/bancorprotocol/contracts .

This security audit has focused on vulnerabilities which could have a negative impact on crowdsale beneficiaries and users interacting with the crowdsale.

Technical Architecture

The crowdsale is primarily centered around a couple of contracts:

  • CrowdsaleChanger
    • is ITokenChanger

The CrowdsaleChanger is the main contract responsible for crowdsale. The purpose is to accept payments in various pre-specified assets, issue tokens and track contributions.

  • EtherToken
    • is ERC20Token
      • is IERC20Token
      • is SafeMath
    • is IEtherToken
      • is IERC20Token

The EtherToken is an ERC20 implementation for Ether, allowing standard handling of ethers as tokens. It has (via parent) protection against sending to null account. The EtherToken is not 'centrally managed', but is a general (and open) contract that anyone can use without trusting a central party.

  • SmartToken
    • is ERC20Token
      • is IERC20Token
      • is SafeMath
    • is Owned
      • is IOwned
    • is ISmartToken
      • is IOwned
      • is IERC20Token

The SmartToken is a somewhat special ERC20Token implementation, which destroys tokens sent to itself. This is the 'native' tokens issued by the crowdsale.

The tokenOwner has high privileges within both contracts (CrowdsaleChanger/SmartToken). If the ownership of SmartToken can be subverted or lost, impact would be high. Risks are mitigated through

  1. ownerOnly can set a newOwner, which is not effectual until
  2. newOwner performs a call to acceptOwnership()

After these two steps, it is certain that the old owner initiated the transfer, and that someone holds a key that can perform contract invocations from the new address.

The SmartToken has two forms of controllers; a changer and an owner. If changer is set, that entity becomes the most powerful user. If changer is not set, owner becomes the primary controller of the token.

However, there is no 'override', by which the owner can take back control from the changer, if the changer is somehow compromised.

Only the token owner can call the following CrowdsaleChanger methods

  • withdraw
  • disableERC20Token
  • updateERC20Token
  • enableRealCap
  • setTokenChanger
  • initERC20Tokens

Previous findings

  • Medium: Missing protection against user errors
    • Fixed via modifier isValidAddress in ERC20Token.sol
  • Low:: SmartToken - possible to issue to this
    • Fixed
  • Low: Potential uninitailized variable (CrowdSaleChanger)
    • fixed
  • Low: Not possible to unset changer (BancorChanger, CrowdsaleChanger)
    • fixed
  • Info: ERC20Token, EtherToken, BancorToken,SmartToken - Interface ambiguity
    • Fixed via updated documentation
  • Info: BancorToken - missing reserve cap
    • N/A (BancorToken removed)
  • Info: BancorToken - withdraw
    • N/A (BancorToken removed)
  • Info: BancorToken - CRR configuration not changeable during setup
    • N/A (BancorToken removed)
  • Info: SmartToken - central point of ownership
    • feature - still present
  • Info: Returnvalus not used
    • fixed
  • Info: constant methods accessing msg.sender (BancorChanger)
    • fixed

Findings

Medium: Possibility for lost funds

There are a number of cases where users mistakenly have sent tokens to various contracts, which have no ability to claim the tokens.

The CrowdsaleChanger has a withdraw function to allow controllers to withdraw ERC20 tokens mistakenly sent to the contract. However, the withdraw function can only be invoked for ERC20 tokens enrolled into the crowdsale. In the event that an 'external' token is mis-sent to the contract, it would be impossible to recover the asset once the crowdsale is active.

It was also noted that there is no corresponding protection mechanisms on SmartToken, nor EtherToken.

Recommendations

  • Add a withdraw functionality to SmartToken and EtherToken
  • Remove validERC20Token(_erc20Token) modifier from CrowdsaleChanger.withdraw

Note: A withdraw functionality on EtherToken requires some form of predefined management account with the ability to withdraw tokens mistakenly sent to the contract.

Info: Hidden cap

The crowdsale is configured to have a hidden cap. The cap is configured during deployment as a hash : realEtherCapHash. The tokenOwner can, during the sale, reveal the hidden cap.

However, there's nothing to ensure that this revelation is actually performed, and failure to do so means that the initial totalEtherCap remains unchanged. This is not a vulnerability, since the motivation behind a hidden cap is to create uncertainty around the cap; an objective which is reached regardless of revelation or not.

Info: Unused code

Modifier never used

    // validates a token address - verifies that the address belongs to one of the changeable tokens
    modifier validToken(address _address) {
        require(_address == address(token) || tokenData[_address].isSet);
        _;
    }

Recommendation

Remove unused code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment