Skip to content

Instantly share code, notes, and snippets.

@holiman

holiman/Bancor-CrowdsaleAudit-followup.md Secret

Last active June 1, 2017 09:47
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save holiman/6d00cddec7aac98a095b9ff22838c40d to your computer and use it in GitHub Desktop.
Save holiman/6d00cddec7aac98a095b9ff22838c40d to your computer and use it in GitHub Desktop.
Download ZIP
Raw
Bancor-CrowdsaleAudit-followup.md

Background and summary

This document is a follow-up 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, and a final report, available here.

This is the fifth document, to follow up on changes since the final report.

Based on the audit, we estimate the risk level of the Bancor crowdsale contract(s) to be Low. The security of all funds reside heavily on the security surrounding the beneficiary account, which is outside the scope of the audit, and is assumed to be a very secure multisig wallet.

Scope

This follow-up report covers the contracts central to the Bancor crowdsale, and is part of a larger audit covering all Bancor contracts. This follow-report was performed on commit hash 715bdde572c540297ce4fc6b46b1ed2b85f55bdc 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

Since the last report, the crowdsale is inheritance structure has been modified. The following are the central contracts for the crowdsale:

  • CrowdsaleController
    • is SmartTokenController
      • is TokenHolder
        • is Owned
          • is IOwned
    • is SafeMath

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

The EtherToken has been removed from the crowdsale, since the last report. Ether is now directly transferred to the beneficiary. This reduces the overall complexity of the crowdsale implementation.

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

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 (CrowdsaleController/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.

Since the last report, the controller has been removed, and there's now only the concept of owner.

Only the token owner can call the following CrowdsaleController methods

  • CrowdsaleController.enableRealCap
  • SmartTokenController.transferTokenOwnership
  • SmartTokenController.acceptTokenOwnership
  • SmartTokenController.disableTokenTransfers
  • SmartTokenController.issueTokens
  • SmartTokenController.destroyTokens
  • SmartTokenController.withdrawFromToken
  • TokenHolder.withdrawTokens

Earlier findings

These findings were submitted in previous reports

  • Medium: Possibility for lost funds (SmartToken/EtherToken)
    • Fixed via TokenHolder parent
  • 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
  • Info : Unused code
    • fixed, code removed
  • Info : Hidden cap
    • Not a security issue - not obligatory to reveal hidden cap

Outstanding issues

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