These document is an audit report about the Bancor protocol contracts, which have been audited by Martin Holst Swende on behalf of Dyno Security AB. During the development process, there have been several preliminary reports with potential issues, which have been addressed during the course of development. Additionally, two previous reports concerning the crowdsale specifically.
The Bancor protocol implementation has a security model based on centralized trust: the owner
of the contracts have, to a large degree, full control over assets traded over the platform. This makes it possible to avoid many problems in fully decentralized solution, where all upgrade paths, invariant-check-based escape hatches etc, need to be defined beforehand.
Since the bancor protocol is fairly a complex scheme involving a large number of interacting contracts, it makes sense to have a centralized model, at least initially.
The security of the account(s) which ultimately secure and control the audited contract is not within scope of this audit.
Based on the audit, we estimate the Bancor contracts have a high degree of security, and estimate the risk level fo the contract(s) to be Low.
The scope of the audit is all contracts within the Bancor repository, with the one exception of BancorFormula.sol - since we've been involved in writing that contract, we cannot perform security audit of it.
This security audit has focused on vulnerabilities which could have a negative impact on crowdsale beneficiaries and users interacting with the crowdsale.
The audit was performed against Bancor Protocol git repository at hash 715bdde572c540297ce4fc6b46b1ed2b85f55bdc
.
The primary contracts are described below.
- BancorChanger
- is ITokenChanger
- is SmartTokenController
- is TokenHolder
- is Owned
- is IOwned
- is Owned
- is TokenHolder
BancorChanger
is the main exchange for various ERC20Tokens, implementing the ITokenChanger
interface. Through SmartTokenController
, it also controls a native token of it's own.
The owner
of BancorChanger
can do a lot of actions on the contract, such as:
-
By using
updateReserve
, the owner can enable and setvirtualBalance
for any token, and thus increase or decrease the reserve at will - e.g. to artifically increase or decrease the token price. -
By using
BancorChanger.withdrawTokens
, the owner can withdraw tokens from the reserve at will.
These two methods are intended as a backup security feature in to allow recovering assets from the contract in case of extreme events, and not for use during normal execution.
- SmartToken.Sol
- is ISmartToken
- is ERC20Token
- 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 used by the BancorChanger. The SmartToken
is fully controlled by the owner
, who is able to issue
and destroy
tokens at will.
- EtherToken
- is ERC20Token
- is IERC20Token
- is SafeMath
- is IEtherToken
- is IERC20Token
- is Owned
- is IOwned
- TokenHolder
- 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. The inherited 'Owned' feature, however, enables the owner
to recover missent tokens, but not otherwise interfere with the contract.
Note: since the EtherToken
is not centrally governed, it is fully possible for Ether to be 'lost' - e.g. if someone were to transfer
to an address with a typo. The owner
cannot, in that case, revert the operation. This is, however, in line with how any normal transfer of Ether works, and does not introduce any new threats for users of the contract.
The TokenHolder
is gives all contracts above protection against tokens that were mistakenly sent directly to the contract.
It appears no longer possible to change the formula after creation. In case the formula needs to be updated,
- a new BancorChanger would have to be instantianted,
- and initialized with the reserves,
- and then set as
owner
of theSmartToken
(which would make the old BancorChanger becomeinactive
, disabling selling and purchasing) - and transfer all reserves to the new
BancorChanger, which can be done through
BancorChanger.withdrawTokens`.
Consider adding back the ability to set the formula without having to redeploy the BancorChanger
.
The BancorChanger can ostensibly be stopped, if the disableTransfers
is called on the SmartToken. The Changer can do this, via it's inherited 'disableTokenTransfers' method from SmartTokenController
.
The transfersAllowed
modifier is only used for :
transferFrom
transfer
and not used for issue
and destroy
. This means that even though the underlying 'native' token is frozen for transfer, the BancorChanger methods buy
and sell
would keep operating.
After discussion with the developers; it appears that the ability to disable transfers of tokens is to prevent trading of tokens during the crowdsale, and not to prevent the BancorChanger
from operating.
The EtherToken
contract manages it's own totalSupply
, which should always match the actual balance
. The only way those would not match, would be if
- Someone sent ether to the contract via
SELFDESTRUCT
- Someone sent ether to the contrat via mining rewards
- There was an error in the calculation of
totalSupply
.
If such an even occurred, the Ether would be trapped, since the extra ether would not be assigned to any msg.sender
.
This is a very unlikely scenarion, hence the rating of 'Info'.
Add a method to extract Ether exceeding the totalSupply
, something along these lines:
function extractLostEther()
{
if( this.balance > totalSupply ){
owner.send(this.balance - totalSupply)
}
}