A CONSENSYS DILIGENCE Audit Report

Balancer Finance

1 Executive Summary

In April 2020, Balancer asked us to conduct a security assessment of Balancer Finance - Balancer core: an automated portfolio manager, liquidity provider, and price sensor.

We performed this assessment from May 4 to May 15, 2020. The assessment primarily focused on the high-level logic of balancer-core: BPool. The engagement was conducted by Alexander Wade and Shayan Eskandari, the total effort spent was 4 person-weeks.

1.1 Scope

Our review focused on the commit hash 5d70da92b1bebaa515254d00a9e064ecac9bd18e. The list of files in scope can be found in the Appendix.

Balancer’s BPool implementation makes use of a set of complicated formulas for interacting with the protocol. The definitions and derivations of these formulas are located in the whitepaper (see below). The EVM implementation of these formulas requires algebraic transformations, exponentiation approximation, and other considerations in order to compute these formulas with reasonable margin of error and gas costs.

The general correctness of these formulas and their implementation was out of scope for this assessment, as the priority for this review was the high-level logic of BPool and its parent contracts.

1.2 Documentation

Alongside an initial code walkthrough provided by the client, the following documentation was available during our assessment:

2 System Overview

Balancer is “a generalized Uniswap” that users can hold tokens in a pool with ratios other than 50-50. The ratios are calculated by the normalized weight of each token in the pool.

Below you can see the visualization of the Balancer system.

alt text

3 Recommendations

During the course of our review, we made the following recommendations:

3.1 Restrict access to setController so that it may only be called before finalization

Description

setController is used to change the privileged _controller address, which is able to perform many administrative actions before calling finalize. After finalization, the _controller serves no purpose.

Locking the function will ensure it is not used, and will reduce confusion for users of the BPool.

Recommendation

Add require(!finalized) to BPool.setController

3.2 Ensure bound and rebound token values are exactly correct

Description

For both BPool.bind and BPool.rebind, the balance parameter is used to determine how many tokens the pool will absorb from msg.sender (or release to msg.sender):

code/contracts/BPool.sol:L286-L297

// Adjust the balance record and actual token balance
uint oldBalance = _records[token].balance;
_records[token].balance = balance;
if (balance > oldBalance) {
    _pullUnderlying(token, msg.sender, bsub(balance, oldBalance));
} else if (balance < oldBalance) {
    // In this case liquidity is being withdrawn, so charge EXIT_FEE
    uint tokenBalanceWithdrawn = bsub(oldBalance, balance);
    uint tokenExitFee = bmul(tokenBalanceWithdrawn, EXIT_FEE);
    _pushUnderlying(token, msg.sender, bsub(tokenBalanceWithdrawn, tokenExitFee));
    _pushUnderlying(token, _factory, tokenExitFee);
}

Because token balance changes can happen outside of the context of this function, an extra check at the bottom would ensure that the rebind operation was performed successfully and with complete understanding of the state of the pool:

require(_records[token].balance == token.balanceOf(address(this)));

Alternatively, consider performing an operation similar to that implemented in gulp:

code/contracts/BPool.sol:L333-L341

// Absorb any tokens that have been sent to this contract into the pool
function gulp(address token)
    external
    _logs_
    _lock_
{
    require(_records[token].bound, "ERR_NOT_BOUND");
    _records[token].balance = IERC20(token).balanceOf(address(this));
}

3.3 Include sanity-check for extcodesize on bound tokens

Description

Generally, users of a BPool should recognize and trust all of the pool’s bound tokens before interacting with it. To help with this somewhat (and ensure addresses are not bound accidentally), an extcodesize check could be added to BPool.bind.

Recommendation

Ensure extcodesize of tokens is nonzero in BPool.bind

3.4 Consider implementing a minimum _totalWeight for unbind and rebind

Description

BPool.rebind and BPool.unbind do not explicitly check that a decrease in _totalWeight results in a usable value. Swaps will not function correctly if _totalWeight moves outside of certain bounds; the MAX_TOTAL_WEIGHT restriction in rebind provides some assurance on the cap of _totalWeight:

code/contracts/BPool.sol:L276-L280

// Adjust the denorm and totalWeight
uint oldWeight = _records[token].denorm;
if (denorm > oldWeight) {
    _totalWeight = badd(_totalWeight, bsub(denorm, oldWeight));
    require(_totalWeight <= MAX_TOTAL_WEIGHT, "ERR_MAX_TOTAL_WEIGHT");

Implementing a minimum value will provide assurance on the lower bound of _totalWeight.

Recommendation

Add a require to rebind and unbind that MIN_WEIGHT * _tokens.length <= _totalWeight

Alternatively, automatically set _publicSwap to false if _totalWeight drops below MIN_WEIGHT.

3.5 Disallow self-bound pools

Description

BPool’s token can be interacted with in much the same way as the rest of the pool’s bound tokens, even if it is not bound. joinPool, exitPool, joinswap*, and exitswap* each allow users to purchase and sell a pool’s own token in exchange for varying quantities of the pool’s bound tokens.

However, BPool’s token can also be bound to its own pool explicitly. In this case, many internal accounting functions do not properly track operations (transfer, mint, burn, etc) performed on pool tokens.

Recommendation

Disallow binding a pool’s token to itself. Add a check in bind:

require(token != address(this));

3.6 Use of modifiers for repeated checks

Description

It is recommended to use modifiers for common checks within different functions. This will result in less code duplication in the given smart contract and adds significant readability into the code base.

Examples

The main suggestion is for, but not limited to, the following checks in BPool.sol contract:

  • require(msg.sender == _controller, "ERR_NOT_CONTROLLER"); has been repeated 7 times in BPool contract, which can be replaced with onlyController() modifier with the same require
  • require(!_finalized, "ERR_IS_FINALIZED"); has been repeated 6 times in the contract, similarly this can be replaced with notFinalized() modifier with the same require
  • require(_finalized, "ERR_NOT_FINALIZED"); has been repeated 7 times in the contract, it can be replaced with finalized() modifier with the same require

3.7 Remove unused code

Description

BColor.sol which includes BColor and BBronze contracts, solely exist to indicate the version of the factory and the pool. BBronze is inherited in many contracts and makes overall contract structure unnecessary complicated.

Inheritance graph

Recommendation

The color (version) can be represented by the something like following line in BConst.sol:

    bytes32 public constant BColor = bytes32("BRONZE");

3.8 PBT unique naming

Description

Currently each pool mints its own token named Balancer Pool Token with the symbol BPT. If tracked on etherscan, all pools show the same token name, but different address, which might be confusing to the users.

Examples

BPTs

Recommendation

Let Pool controller name their Pool share token.

3.9 Inconsistent require checks in AmountIn & AmountOut

Description

The main difference between *AmountIn and *AmountOut are that one checks the lower bound price using minAmountOut and the other the maximum price using maxPoolAmountIn, reflectively for “buy” and “sell” tokens.

However, the checks in some of these functions are inconsistent.

Example

code/contracts/BPool.sol:L595-L605

tokenAmountIn = calcSingleInGivenPoolOut(
                    inRecord.balance,
                    inRecord.denorm,
                    _totalSupply,
                    _totalWeight,
                    poolAmountOut,
                    _swapFee
                );

require(tokenAmountIn != 0, "ERR_MATH_APPROX");
require(tokenAmountIn <= maxAmountIn, "ERR_LIMIT_IN");

The equivalent non-zero check from the above code snippet is missing in the joinswapExternAmountIn function below:

code/contracts/BPool.sol:L562-L572

poolAmountOut = calcPoolOutGivenSingleIn(
                    inRecord.balance,
                    inRecord.denorm,
                    _totalSupply,
                    _totalWeight,
                    tokenAmountIn,
                    _swapFee
                );

require(poolAmountOut >= minPoolAmountOut, "ERR_LIMIT_OUT");

The check happens implicitly by the following line, but none of the checked values had a non-zero check beforehand.

require(poolAmountOut >= minPoolAmountOut, "ERR_LIMIT_OUT");

Recommendation

Verify all the checks in similar functions.

Also based on the code similarity in the *AmountIn and *AmountOut functions, there might be a better way to implement these pair functions and merge them together. The solution is yet to be discussed and can be implemented on future versions of Balancer.

3.10 Perform more rigorous input validation across swap functions

Description

Several functions could use additional input validation checks. Generally, many functions tend to allow trades with nonsensical input and output values, which may exposes edge-case behavior.

The following examples provide several locations where additional input validation should be performed:

Examples

  1. joinPool and exitPool should both check that maxAmountsIn and minAmountsOut have equivalent length to BPool._tokens

  2. swapExactAmountIn and swapExactAmountOut should check that tokenIn != tokenOut

  3. swapExactAmountIn and swapExactAmountOut should check that both spotPriceBefore and spotPriceAfter are nonzero.

  4. swapExactAmountIn should check that tokenAmountOut != 0

  5. swapExactAmountOut should check that tokenAmountIn != 0

  6. joinswapExternAmountIn should check that tokenAmountIn != 0 and that poolAmountOut != 0

  7. joinswapPoolAmountOut should check that poolAmountOut != 0

  8. exitswapPoolAmountIn should check that poolAmountIn != 0 and that tokenAmountOut != 0

  9. exitswapExternAmountOut should check that tokenAmountOut != 0

Recommendation

Add the aforementioned sanity checks to all trade functions.

Additionally, reject trades where “zero tokens” are either the input or the output.

4 Security Specification

This section describes, from a security perspective, the expected behavior of the system under audit. It is not a substitute for documentation. The purpose of this section is to identify specific security properties that were validated by the audit team.

4.1 Actors

The relevant actors are listed below with their respective abilities:

  • BLabs: BFactory owner The address deploying BFactory

    • Can change the BLabs address
    • Can collect factory fees from pools

  • Pool Controller: Each pool has an address associated with it as Controller, which is the address calling newBPool() in the BFactory contract

    • Can change the controller address
    • Can set SwapFee, which is enforced to be between MIN_FEE and MAX_FEE (Defined in BConst as 0.0001% and 10% respectively)
    • Can switch publicSwap, given that the pool is not finalized yet
    • Can Finalize the pool, which will make the pool public and joinable for others
    • Can bind, rebind, and unbind tokens to the pool (up to 8 tokens for each pool), and set the weights of each token. This is only possible when the pool is not finalized yet

  • Anyone: Any other ethereum address

    • Can update the balance of the tokens in the pool by calling gulp()
    • Can Join and Exit any finalized pool and deposit tokens based on their max prices
    • Can Swap Pool token, and individual tokens

4.2 Trust Model

In any smart contract system, it’s important to identify what trust is expected/required between various actors. For this audit, in addition to Actors section, we established the following trust model:

  • It is important for anyone willing to join a pool to make sure all the tokens bound to that pool are recognized and verified. Many functionalities in the pool, such as Join Pool, Exit Pool, and Swap functions, do external calls to the tokens contracts and it is assumed that the bound tokens are safe to interact with.
    • Any upgradable tokens must be verified before each call to the pool.
  • Pool Exit fee is currently set to 0 in BConst.sol, however the code exist to send the fees to the factory on rebinding tokens or exiting pool.
  • On joining the pool, a maximum token amount maxAmountsIn is passed to protect user from high price fluctuation that may be caused by front-running or other users. These values should be correctly calculated and visible in the user interface.
  • The mathematic formulas implemented in BMath.sol and BNum.sol follow the formulas in the Balancer whitepaper. However their implementations are restricted by Solidity limits. Same as issue 5.1, more rounding issues might exist and requires further unit tests for edge cases.
  • As noted in the documentation, Balancer Pools only supports ERC-20 implementations that return Boolean for transfer(), transferFrom(), and other functionalities.

5 Issues

Each issue has an assigned severity:

  • Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
  • Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
  • Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
  • Critical issues are directly exploitable security vulnerabilities that need to be fixed.

5.1 Similar token-to-token swap methods can yield very different results Medium

Description

BPool’s interface exposes several methods to perform token swaps. Because the formula used to calculate trade values varies depending on the method, we compared token swaps performed using two different methods:

  1. BPool.swapExactAmountIn performs a direct token-to-token swap between two bound assets within the pool. Some amount tokenAmountIn of tokenIn is directly traded for some minimum amount minAmountOut of tokenOut. An additional parameter, maxPrice, allows the trader to specify the maximum amount of slippage allowed during the trade.

  2. BPool.joinswapExternAmountIn allows a trader to exchange an amount tokenAmountIn of tokenIn for a minimum amount minPoolAmountOut of the pool’s token. A subsequent call to BPool.exitswapPoolAmountIn allows a trader to exchange amount poolAmountIn of the pool’s tokens for a minimum amount minAmountOut of tokenOut.

While the latter method performs a swap by way of the pool’s token as an intermediary, both methods can be used in order to perform a token-to-token swap. Our comparison between the two tested the relative amount tokenAmountOut of tokenOut between the two methods with a variety of different parameters.

Examples

Each example made use of a testing contract, found here: https://gist.github.com/wadeAlexC/12ee22438e8028f5439c5f0faaf9b7f7

Additionally, BPool was modified; unneeded functions were removed so that deployment did not exceed the block gas limit.

1. tokenIn weight: 25 BONE

tokenOut weight: 25 BONE

tokenIn, tokenOut at equal balances (50 BONE)

tokenAmountIn: 1 BONE

swapExactAmountIn tokenAmountOut: 980391195693945000

joinswapExternAmountIn + exitSwapPoolAmountIn tokenAmountOut: 980391186207949598

Result: swapExactAmountIn gives 1.00000001x more tokens

2. tokenIn weight: 1 BONE

tokenOut weight: 49 BONE

tokenIn, tokenOut at equal balances (50 BONE)

tokenAmountIn: 1 BONE

swapExactAmountIn tokenAmountOut: 20202659955287800

joinswapExternAmountIn + exitSwapPoolAmountIn tokenAmountOut: 20202659970818843

Result: joinswap/exitswap gives 1.00000001x more tokens

3. tokenIn weight: 25 BONE

tokenOut weight: 25 BONE

tokenIn, tokenOut at equal balances (1 BONE)

tokenAmountIn: 0.5 BONE

swapExactAmountIn tokenAmountOut: 333333111111037037

joinswapExternAmountIn + exitSwapPoolAmountIn tokenAmountOut: 333333055579388951

Result: swapExactAmountIn gives 1.000000167x more tokens

4. tokenIn weight: 25 BONE

tokenOut weight: 25 BONE

tokenIn, tokenOut at equal balances (30 BONE)

tokenAmountIn: 15 BONE

swapExactAmountIn tokenAmountOut: 9999993333331111110

joinswapExternAmountIn + exitSwapPoolAmountIn tokenAmountOut: 9999991667381668530

Result: swapExactAmountIn gives 1.000000167x more tokens

The final test raised the swap fee from MIN_FEE (0.0001%) to MAX_FEE (10%):

tokenIn weight: 25 BONE

tokenOut weight: 25 BONE

tokenIn, tokenOut at equal balances (30 BONE)

tokenAmountIn: 15 BONE

swapExactAmountIn tokenAmountOut: 9310344827586206910

joinswapExternAmountIn + exitSwapPoolAmountIn tokenAmountOut: 9177966102628338740

Result: swapExactAmountIn gives 1.014423536x more tokens

Recommendation

Our final test showed that with equivalent balances and weights, raising the swap fee to 10% had a drastic effect on relative tokenAmountOut received, with swapExactAmountIn yielding >1.44% more tokens than the joinswap/exitswap method.

Reading through Balancer’s provided documentation, our assumption was that these two swap methods were roughly equivalent. Discussion with Balancer clarified that the joinswap/exitswap method applied two swap fees: one for single asset deposit, and one for single asset withdrawal. With the minimum swap fee, this double application proved to have relatively little impact on the difference between the two methods. In fact, some parameters resulted in higher relative yield from the joinswap/exitswap method. With the maximum swap fee, the double application was distinctly noticeable.

Given the relative complexity of the math behind BPools, there is much that remains to be tested. There are alternative swap methods, as well as numerous additional permutations of parameters that could be used; these tests were relatively narrow in scope.

We recommend increasing the intensity of unit testing to cover a more broad range of interactions with BPool’s various swap methods. In particular, the double application of the swap fee should be examined, as well as the differences between low and high swap fees.

Those using BPool should endeavor to understand as much of the underlying math as they can, ensuring awareness of the various options available for performing trades.

5.2 Commented code exists in BMath Minor

Description

There are some instances of code being commented out in the BMath.sol that should be removed. It seems that most of the commented code is related to exit fee, however this is in contrast to BPool.sol code base that still has the exit fee code flow, but uses 0 as the fee.

Examples

code/contracts/BMath.sol:L137-L140

uint tokenInRatio = bdiv(newTokenBalanceIn, tokenBalanceIn);

// uint newPoolSupply = (ratioTi ^ weightTi) * poolSupply;
uint poolRatio = bpow(tokenInRatio, normalizedWeight);

code/contracts/BMath.sol:L206-L209

uint normalizedWeight = bdiv(tokenWeightOut, totalWeight);
// charge exit fee on the pool token side
// pAiAfterExitFee = pAi*(1-exitFee)
uint poolAmountInAfterExitFee = bmul(poolAmountIn, bsub(BONE, EXIT_FEE));

And many more examples.

Recommendation

Remove the commented code, or address them properly. If the code is related to exit fee, which is considered to be 0 in this version, this style should be persistent in other contracts as well.

5.3 Max weight requirement in rebind is inaccurate Minor

Description

BPool.rebind enforces MIN_WEIGHT and MAX_WEIGHT bounds on the passed-in denorm value:

code/contracts/BPool.sol:L262-L274

function rebind(address token, uint balance, uint denorm)
    public
    _logs_
    _lock_
{

    require(msg.sender == _controller, "ERR_NOT_CONTROLLER");
    require(_records[token].bound, "ERR_NOT_BOUND");
    require(!_finalized, "ERR_IS_FINALIZED");

    require(denorm >= MIN_WEIGHT, "ERR_MIN_WEIGHT");
    require(denorm <= MAX_WEIGHT, "ERR_MAX_WEIGHT");
    require(balance >= MIN_BALANCE, "ERR_MIN_BALANCE");

MIN_WEIGHT is 1 BONE, and MAX_WEIGHT is 50 BONE.

Though a token weight of 50 BONE may make sense in a single-token system, BPool is intended to be used with two to eight tokens. The sum of the weights of all tokens must not be greater than 50 BONE.

This implies that a weight of 50 BONE for any single token is incorrect, given that at least one other token must be present.

Recommendation

MAX_WEIGHT for any single token should be MAX_WEIGHT - MIN_WEIGHT, or 49 BONE.

5.4 Switch modifier order in BPool Minor

Description

BPool functions often use modifiers in the following order: _logs_, _lock_. Because _lock_ is a reentrancy guard, it should take precedence over _logs_. See example:

pragma solidity ^0.5.0;
pragma experimental ABIEncoderV2;

contract Target {
    
    string[] arr;
    
    modifier a() {
        // sA1
        arr.push("sA1");
        _;
        // sA2
        arr.push("sA2");
    }
    
    modifier b() {
        // sB1
        arr.push("sB1");
        _;
        // sB2
        arr.push("sB2");
    }
    
    // sA1 -> sB1 -> func -> sB2 -> sA2
    function test() public a b {
        arr.push("func");
    }
    
    function get() public view returns (string[] memory) {
        return arr;
    }
}

Recommendation

Place _lock_ before other modifiers; ensuring it is the very first and very last thing to run when a function is called.

6 Document Change Log

Version Date Description
1.0 2020-05-15 Initial report

Appendix 2 - Files in Scope

This audit covered the following files:

File Name SHA-1 Hash
contracts/BFactory.sol 0d193312bc81d4b96c468ae51b6dd27550b8e5ae
contracts/BPool.sol 04450c7c1e9d861475cd1e1d673b992c810af756
contracts/BToken.sol 2447c07499a00d39a5aec76b68c6d5d58928d64d
contracts/BNum.sol f679764be21d158411032bfad7f658210058c4ca
contracts/BConst.sol 459521a827d8302be1fd6c16b77721aea8ef24a1
contracts/BColor.sol 6fc688e13f12d4dbff1aa44de0e1203b1e1dbdd9
contracts/BMath.sol c5cde402b16dd6ea0263ec626ae559de370a1ddb

Appendix 3 - Artifacts

This section contains some of the artifacts generated during our review by automated tools, the test suite, etc. If any issues or recommendations were identified by the output presented here, they have been addressed in the appropriate section above.

A.3.1 MythX

MythX is a security analysis API for Ethereum smart contracts. It performs multiple types of analysis, including fuzzing and symbolic execution, to detect many common vulnerability types. The tool was used for automated vulnerability discovery for all audited contracts and libraries. More details on MythX can be found at mythx.io.

Below is the raw output of the MythX vulnerability scan:

A.3.2 Ethlint

Ethlint is an open source project for linting Solidity code. Only security-related issues were reviewed by the audit team.

Below is the raw output of the Ethlint vulnerability scan:

A.3.3 Surya

Surya is a utility tool for smart contract systems. It provides a number of visual outputs and information about the structure of smart contracts. It also supports querying the function call graph in multiple ways to aid in the manual inspection and control flow analysis of contracts.

Below is a complete list of functions with their visibility and modifiers:

A.3.4 Tests Suite

Below is the output generated by running the test suite:

Appendix 4 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) – on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.