-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
- 1 - Table of Contents
- 2 - Introduction
- 2.1 - Authenticity
- 2.2 - Audit Goals and Focus
- 2.2.1 - Sound Architecture
- 2.2.2 - Smart Contract Best Practices
- 2.2.3 - Code Correctness
- 2.2.4 - Code Quality
- 2.2.5 - Security
- 2.2.6 - Testing and testability
- 2.3 - About Bancor
- 2.4 - Terminology
- 2.4.1 - Likelihood
- 2.4.2 - Impact
- 2.4.3 - Severity
- 3 - Overview
- 3.1 - Source Code
- 3.1.1 - First Audit
- 3.1.2 - Second Audit
- 3.2 - General Notes
- 3.3 - Contracts
- 4 - First audit findings
- 4.1 - Note Issues
- 4.2 - Low Issues
- 4.3 - Medium Issues
- 4.4 - High Issues
- 4.5 - Critical Issues
- 5 - Second audit findings
- 5.1 - Note Issues
- 5.2 - Low Issues
- 4.3 - Medium Issues
- 4.4 - High Issues
- 4.5 - Critical Issues
From 2017-05-17 to 2017-05-26, Nick Johnson performed an audit of the Bancor smart contracts. My findings are detailed below.
On 2017-05-30, I performed a second audit of the Bancor smart contracts, also detailed below.
I, Nick Johnson have no stake or vested interest in Bancor. This audit was performed under a contracted hourly rate with no other compensation.
This document should have an attached cryptographic signature to ensure it has not been tampered with. The signature can be verified using the public key from Nick Johnson's keybase.io record.
This audit includes assessments of the overall architecture and design choices. Given the subjective nature of these assessments it will be up to the Bancor development team to determine whether any changes should be made.
This audit will evaluate whether the codebase follows the current established best practices for smart contract development.
This audit will evaluate whether the code does what it is intended to do.
This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.
This audit will look for any exploitable security vulnerabilities, or other potential threats to either the operators of Bancor or its users.
This audit will examine how easily tested the code is, and review how thoroughly tested the code is.
Bancor is a smart-contract-based token conversion protocol, which enables a single party to convert any token to another, without requiring a second party to exchange with. It achieves this through the use of reserve-tokens, which provide liquidity through autonomous algorithmic price discovery, regardless of trade volume.
This audit uses the following terminology.
How likely a bug is to be encountered or exploited in the wild, as specified by the OWASP risk rating methodology.
The impact a bug would have if exploited, as specified by the OWASP risk rating methodology.
How serious the issue is, derived from Likelihood and Impact as specified by the OWASP risk rating methodology.
The Bancor smart contract source code was made available in the bancorprotocol/contracts Github repository.
The code was audited as of commit 2ae8ea8b71221a5d9e27b0a376a3d3abed26de73.
The following Solidity source files (with SHA1 sums) were audited:
SHA1(solidity/contracts/BancorChanger.sol)= 9b89bf31aa8a56b7103e964730fe69555e4ad1f3
SHA1(solidity/contracts/CrowdsaleChanger.sol)= 8218a3eb91615eb0dc40a430e32d8f1951b7a2b3
SHA1(solidity/contracts/ERC20Token.sol)= a78e208650a8fc74a295de3ce0cbf869f6bf8257
SHA1(solidity/contracts/ERC20TokenInterface.sol)= aa00d831403f2b4b6ed277f1efc1b3df36f6b020
SHA1(solidity/contracts/EtherToken.sol)= fcf4f5a02b79054e9f8ed3f2cd21555e71dda8bd
SHA1(solidity/contracts/Owned.sol)= 124934782a4dad37163be1e0c6cf5a1575af7401
SHA1(solidity/contracts/SafeMath.sol)= 821ff3a9a9d86fed5b29231ce79a249f103e04cd
SHA1(solidity/contracts/SmartToken.sol)= c3cda707b91436fee2ad3aa09132da4d05853c7f
SHA1(solidity/contracts/SmartTokenInterface.sol)= 98572ed194b52c948b2f53d2bde18c1977a0de4e
SHA1(solidity/contracts/TokenChangerInterface.sol)= a64fc5c81906d7f3dedb08106fdd25aed5454ad8
BancorFormula.sol was NOT audited.
The code was audited as of commit 715bdde572c540297ce4fc6b46b1ed2b85f55bdc.
The following Solidity source files (with SHA1 sums) were audited:
SHA1(solidity/contracts/BancorChanger.sol)= c88d93a6db762772cac69e93272d20c6810f88c6
SHA1(solidity/contracts/CrowdsaleController.sol)= 40a2554385450500dee6dfe57ff76209ff653ec1
SHA1(solidity/contracts/ERC20Token.sol)= 5ecd041a35833cf4476a4bc187e45df897d36508
SHA1(solidity/contracts/EtherToken.sol)= e5660b29350218fc1829b28bdfa3acbc08245fe1
SHA1(solidity/contracts/IBancorFormula.sol)= c6b8f4ba91323779789dee084a7c6c014f3b8aba
SHA1(solidity/contracts/IERC20Token.sol)= 74e7414b3a97743096feb6667c214315e9d2e68f
SHA1(solidity/contracts/IEtherToken.sol)= 4046b6ad26c80f3e29bd46a0b8a24407f2d5a009
SHA1(solidity/contracts/IOwned.sol)= 8047485cb1fc395247e3c65acd95cb330b66c692
SHA1(solidity/contracts/ISmartToken.sol)= a549cfc935518e5de3249bb6e75d7ea99191a68c
SHA1(solidity/contracts/ITokenChanger.sol)= a3b82e10db97e1bf852a06ccb11360daeaf90f50
SHA1(solidity/contracts/ITokenHolder.sol)= bd695ed58b06d5d373dedd3141dfdb2335006abc
SHA1(solidity/contracts/Owned.sol)= 3f88cc2e1a5eec64037fbe50c51c86f0e39cd4cf
SHA1(solidity/contracts/SafeMath.sol)= f5146f2c6a21801734442d085fcd3d1c89a3a399
SHA1(solidity/contracts/SmartToken.sol)= 3b0f68b461d2c45490715d8192a9247720f57415
SHA1(solidity/contracts/SmartTokenController.sol)= 4958076021dbb1a924e8bf61c3d852c099fd81a2
SHA1(solidity/contracts/TokenHolder.sol)= 2823c42206f343dd8de41ec5d05034908e7b5ecb
BancorFormula.sol was NOT audited.
Overall, the code is clearly written, and demonstrates effective use of abstraction, separation of concerns, and modularity. Modifiers are used extensively throughout, and other validation is frontloaded with require and assert statements that make it easy to assess the conditions under which a function can execute. The design of the contracts show attention to detail and succeed in reducing or eliminating nonobvious edge cases.
This significantly aided auditing, and should make it easier for readers to understand the functionality of the contract. Other smart contract authors would be well advised to study this code as an example of how to write good smart contract code in Solidity.
Participants should note that the contracts as authored for the crowdsale are not trustless, and depend on the good behaviour of Bancor. Bancor have stated that this is intentional, intended to allow them to respond to and remedy any issues that come up during the crowdsale and in early operation, and that manual oversight will be exchanged for more automated operation once they are confident the system is working as intended.
We strongly recommend that all 'owner' addresses during this initial phase should be governed by a multisig, preferably with significant oversight and participation by nonaffiliated individuals.
The code is well modularised. Owned and SafeMath provide basic utility functionality used throughout the code.
ERC20TokenInterface defines an interface for ERC20 compatible tokens, and ERC20Token provides an implementation of this interface.
EtherToken provides an ERC20Token implementation used for tokenising ether. This is used inside the CrowdsaleChanger and BancorChanger for consistency, so as to not require special handling for Ether.
SmartToken builds on ERC20Token by adding 'owner' and 'changer' mechanisms, as well as facilities for owners and changers to issue and destroy tokens. When a changer is set, the owner no longer has priveliged control over the contract. SmartTokenInterface provides an interface for this contract.
TokenChangerInterface provides an interface for changers, as used by SmartToken. A changer is capable of issuing SmartTokens in exchange for some other form of token, and forms the basis of the Bancor system.
Two implementations of TokenChangerInterface are provided. CrowdsaleChanger provides a simple implementation that exchanges tokens for ether and a variety of other tokens at a fixed ratio, to be used during the crowdfunding portion.
BancorChanger implements Bancor's core functionality, providing for exchanging between a set of reserve tokens and a SmartToken using a variable exchange rate that adjusts in response to demand.
BancorFormula (not audited here) implements the exchange formula specified for Bancor, and is referenced by BancorChanger to determine the exchange rate between different token types.
The tests are generally well written and complete, covering both happy paths and a wide range of exceptions and edge cases. Tests are easy to read and follow.
However, running the tests is currently unintuitive, requiring manually starting testrpc with a custom command line, and running the tests in a separate terminal. This does not appear to be documented anywhere. There is also no automated testing or continuous build for the repository, which increases the risk of a change introducing an undetected bug that would have been caught by tests.
We strongly recommend making this a single command, and setting up an automated build on the repository to run the tests on each commit.
We found 3 Medium issues, 4 Low issues, and 3 Note issues. There were no High or Critical issues.
Of the issues we found, some, including all the note issues, are subjective and a matter of coding style and clarity. Two of the medium issues represent low likelihood edge cases that we nevertheless believe deserve attention due to the potential for user loss of funds should they manifest.
None of the issues identified represent security vulnerabilities or an undue advantage to an attacker, with the exception of the frontrunning issue identified in section 4.2.5. This issue is common to all onchain exchanges, and countermeasures are difficult for any exchange, though on talking to the Bancor team they have suggestions for future improvements. Bancor have somewhat mitigated frontrunning by requiring users to specify a minimum price at which a trade will be executed, to hedge against large price shifts.
- Likelihood: low
- Impact: low
In several places, return values or argument values of type address are used when a contract type could be used instead.
For instance, SmartTokenInterface.changer() returns an address. Returning a TokenChangerInterface instead
would not change the external ABI, but would allow Solidity-based callers to do additional type
checking, as well as removing the need for a cast from calling code.
In BancorChanger, this gains extra relevance, as _reserveToken and reserveToken are different types.
Other instances of this include SmartTokenInterface.setChanger.
- Likelihood: low
- Impact: low
CrowdsaleChanger defines its own EtherToken interface, which EtherToken does not inherit from. This could lead to changes in EtherToken getting out of sync with contracts that use it.
- Likelihood: low
- Impact: low
Both these interfaces provide access to the same data, and may result in confusion, as well as ABI and code bloat. One or the other should be made non-public.
- Likelihood: medium
- Impact: low
The ERC20Token contract implements the name, symbol and version properties, which are de-facto standards for tokens, but does not implement the decimals property as implemented by, eg, Consensys's HumanStandardToken contract, and as supported by Mist. As a result, Mist and other token browsers may not correctly show the number of decimals for a token.
- Likelihood: medium
- Impact: low
EtherToken implements deposit and withdraw events, but unlike the SmartToken, does not emit any events
when tokens are created or destroyed. Both for consistency and usability, it would make sense to add these events to EtherToken as well, and to name the methods and the events the same as on SmartToken.
- Likelihood: medium
- Impact: low
SmartToken includes a numDecimalUnits field, which is intended by the authors to tell clients how many decimal places to display - rather than the divisor between base units and display units as is commonly used in tokens. We believe this should be removed, as it could result in incorrect implementations by clients leading to lost funds.
- Likelihood: low
- Impact: medium
The contracts contain many multi-line if statements without braces. These are easily missed when editing code and can lead to code that gets executed when not expected; for instance:
if(foo)
one();
two();
We recommend adding braces to these if statements to avoid confusion or bugs introduced when modifying code later.
- Likelihood: low
- Impact: high
SmartToken implements functionality that burns tokens if transferred to the address of the token itself. While this seems a sensible default, absent a compelling need for users to be able to burn tokens, we recommend that this functionality be removed, and the token instead throw an exception in this case. User error, and mistakenly entering the token address in the too field, could otherwise easily lead to them inadvertently destroying tokens.
- Likelihood: low
- Impact: high
CrowdsaleChanger expends significant logic implementing functionality allowing it to accept payment during the crowdsale in a variety of different existing tokens. These tokens are sent to the crowdsale beneficiary account, and will be liquidated for Ether to fund the Bancor Foundation and the reserve at the end of the crowdsale. Exchange rates between tokens and ether are maintained manually by the owner account during the crowdsale period.
We believe that any additional benefit from making it easier for token holders to participate in the crowdsale is outweighed by the technical risk imposed by the additional complexity, and the exchange rate risk imposed by the possibility of price differences between that offered to depositors and that realised when the tokens are sold, and recommend that the crowdsale be simplified to accept only ether.
- Likelihood: medium
- Impact: medium
Because trades are posted publicly to the transaction pool before being mined, other participants in the network have the opportunity to use this information before the transaction takes effect, by submitting their own transaction with a higher gas price, to ensure it is mined before the target transaction. Miners have even more power here, as they have total control over transaction inclusion and ordering.
This is a problem common to all onchain exchanges, and robust solutions are difficult. Bancor have mediated this issue by allowing trades to specify a 'minimum return' below which the trade will not be executed, which permits users to be sure that the price will not shift substantially between their issuing a trade and it executing. They have further outlined future improvements that will reduce the possibility for profit from frontrunning trades.
None found.
None found.
At the time of the second audit, most of our findings from the first audit had been remedied, and no new ones were found. We found 1 note issue, 1 low issue, and 2 medium issues.
One of the medium issues represents a low likelihood edge case that we nevertheless believe deserve attention due to the potential for user loss of funds should it manifest.
None of the issues identified represent security vulnerabilities or an undue advantage to an attacker, with the exception of the frontrunning issue identified in section 5.2.2. This issue is common to all onchain exchanges, and countermeasures are difficult for any exchange, though on talking to the Bancor team they have suggestions for future improvements. Bancor have somewhat mitigated frontrunning by requiring users to specify a minimum price at which a trade will be executed, to hedge against large price shifts.
- Likelihood: low
- Impact: low
Both these interfaces provide access to the same data, and may result in confusion, as well as ABI and code bloat. One or the other should be made non-public.
- Likelihood: low
- Impact: medium
The contracts contain many multi-line if statements without braces. These are easily missed when editing code and can lead to code that gets executed when not expected; for instance:
if(foo)
one();
two();
We recommend adding braces to these if statements to avoid confusion or bugs introduced when modifying code later.
- Likelihood: low
- Impact: high
SmartToken implements functionality that burns tokens if transferred to the address of the token itself. While this seems a sensible default, absent a compelling need for users to be able to burn tokens, we recommend that this functionality be removed, and the token instead throw an exception in this case. User error, and mistakenly entering the token address in the too field, could otherwise easily lead to them inadvertently destroying tokens.
- Likelihood: medium
- Impact: medium
Because trades are posted publicly to the transaction pool before being mined, other participants in the network have the opportunity to use this information before the transaction takes effect, by submitting their own transaction with a higher gas price, to ensure it is mined before the target transaction. Miners have even more power here, as they have total control over transaction inclusion and ordering.
This is a problem common to all onchain exchanges, and robust solutions are difficult. Bancor have mediated this issue by allowing trades to specify a 'minimum return' below which the trade will not be executed, which permits users to be sure that the price will not shift substantially between their issuing a trade and it executing. They have further outlined future improvements that will reduce the possibility for profit from frontrunning trades.
None found.
None found. -----BEGIN PGP SIGNATURE----- Version: Keybase OpenPGP v2.0.71 Comment: https://keybase.io/crypto
wsFcBAABCgAGBQJZLptcAAoJEG2WSZMvKV0Sc04P/2N1Rhacz908OeqsPEmGrI66 GpJVk2llYIiZo0SI4XERNl6xZTCY2Hn5WZSDqQ8Ja3ZibR1htiO6AP8/LnyYkk2V lnS+9Bj89b70MoWiJS6O9PhY71q/NOPHCydVMFPjoaCzBW4aeKupFNsxbQwNVf/g H8pa933RC5zxNoa5c/ZINl0lT4p63CQ93t/nUZmWjw0ughNcq/cT6UzvgroDJFVR YGV1XLX9/1p499cCHIDH5IJT2U6u2wYew2Xcbbl5vro5zD/+SExOUzXz5QJmHGBC n3HzPaUJkbwkpIypF2yM3PANV3kc0aXyjRnYfFaxbIYnLa/TOHKbuK3jH8rU3Kav alb95VDytCeemwl+0zfuiNGgJ9mED+sFjvu/hmuKtvrjIRRGSYyju9fEGK6cP5BO rfdmmbehAtpDoTszT8W+y3RNCWiGHrc6sqcFq3sb11m0y/Ez9bHWq8YlNLDXFkya YwV+2Ts6u+YfIrUUEvusJb77h3pi5CGwzJGHSHCb9iSOx+I1MR3G5UsBy7yzbV+m w40x5PltT0N8AoH8dOiBCXgqma1leTwmZXVrWivyKGSH4lMshJ/CkAA1wlV0Ju6B qJH2+E2pPjzOwL8dm3RZVtM2MRJK5QxrUCXXPl9rpo3YJbDoMtqGy3pL8EQHiJEy ndudIyoP5drykauOUdPG =mikk -----END PGP SIGNATURE-----