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
orcrowdsale
, 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, viadestroy
andissue
- 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
andtokenOwner
, 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 tokensbeneficiary
owns all underlying assets
Based on the audit, we estimate the risk level of the Bancor crowdsale contract(s) to be Low.
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.
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
- is ERC20Token
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
- is ERC20Token
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
ownerOnly
can set anewOwner
, which is not effectual untilnewOwner
performs a call toacceptOwnership()
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
- Medium: Missing protection against user errors
- Fixed via modifier
isValidAddress
in ERC20Token.sol
- Fixed via modifier
- 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 accessingmsg.sender
(BancorChanger)- fixed
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
.
- Add a
withdraw
functionality toSmartToken
andEtherToken
- Remove
validERC20Token(_erc20Token)
modifier fromCrowdsaleChanger.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.
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);
_;
}
Remove unused code.