Missing checks allow strategists to steal all fund via tailOff

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L533-L563
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L343-L369

Vulnerability details

Impact

Strategists can call tailOff with malicious payload to steal all funds within any BathToken.

There are 2 issues that makes this possible:

• BathPair.tailOff allows arbitrary _stratUtil address.
• BathToken.rebalance allows underlying token as filledAssetToRebalance.

These allow malicious strategists to input any token address, including the underlying token of a BathToken, and transfer them to a contract of their choosing.

Proof of Concept

A malicious strategist calls tailOff with the following payload:

{
    targetPool: bathUSDC.address,
    tokenToHandle: USDC.address,
    targetToken: USDT.address, // any address
    _stratUtil: maliciousContract.address,
    amount: USDC.balanceOf(bathUSDC.address),
    hurdle: 0, // any
    _poolFee: 0 // any
}

bathUSDC BathToken will then send all USDC to the strategist's maliciousContract. All deposits are lost.

Recommended Mitigation Steps

• Whitelist the addresses that can be used as _stratUtil.
• Add a check in rebalance to prevent transferring underlying token:

  require(filledAssetToRebalance != underlyingToken, "must not be underlying");
  

[HickupHH3]

Making this the primary issue of strategist stealing funds via the tailOff() function. Rationale of med severity is outlined in #344.

[bghughes]

Good issue, implemented the require(filledAssetToRebalance != underlyingToken, "must not be underlying"); fix 💯