Skip to content

Instantly share code, notes, and snippets.

@holiman holiman/BancorAudit.md Secret
Last active Jun 20, 2017

Embed
What would you like to do?

Summary and background

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.

Scope

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.

Technical architecture

The primary contracts are described below.

  • BancorChanger
    • is ITokenChanger
    • is SmartTokenController
      • is TokenHolder
        • is Owned
          • is IOwned

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 set virtualBalance 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

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.

Findings

  • First report on Crowdsale contracts: here.
  • Follow-up report on Crowdsale contracts here

Info: Not possible to set formula

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 the SmartToken (which would make the old BancorChanger become inactive, disabling selling and purchasing)
  • and transfer all reserves to the new BancorChanger, which can be done through BancorChanger.withdrawTokens`.

Recommendation:

Consider adding back the ability to set the formula without having to redeploy the BancorChanger.

Info: 'transfer' even though transfers disabled

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.

Info: Missing ability to recover trapped ether (EtherToken.sol)

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'.

Recommendation

Add a method to extract Ether exceeding the totalSupply, something along these lines:

function extractLostEther()
{
	if( this.balance > totalSupply ){
		owner.send(this.balance - totalSupply)
	}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.