Rubicon contest - horsefacts's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L202 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L274 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L366 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L419 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L251 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L406 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L377 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L320 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L303 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L251

Vulnerability details

The return values of ERC20 transfer and transferFrom are not consistently checked throughout the codebase. Tokens that return false rather than revert to indicate failed transfers may silently fail rather than reverting as expected.

In addition, since ERC20 is used directly for many transfers (rather than a safe transfer helper), transferring weird ERC20s with missing return values may revert in unexpected cases.

-RubiconRouter#swap -RubiconRouter#swapEntireBalance -RubiconRouter#buyAllAmountForETH -RubiconRouter#offerForETH -RubiconRouter#_swap -RubiconRouter#offerWithETH -RubiconRouter#buyAllAmoutForETH -RubiconRouter#maxSellAllAmount -RubiconRouter#maxBuyAllAmount -RubiconRouter#_swap

Recommendation: Use a safe transfer library like OpenZeppelin SafeERC20 to ensure consistent handling of ERC20 return values and abstract over inconsistent ERC20 implementations.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L381-L409 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L453-L472 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L494-L517

Vulnerability details

The RubiconRouter offerWithETH, depositWithETH and swapWithETH functions all accept ETH transfers greater than or equal to the expected payment amount. Since the excess amount is not deposited or refunded, it will be locked in the router contract if a caller sends excess ETH.

RubiconRouter#offerWithETH

    // Pay in native ETH
    function offerWithETH(
        uint256 pay_amt, //maker (ask) sell how much
        // ERC20 nativeETH, //maker (ask) sell which token
        uint256 buy_amt, //maker (ask) buy how much
        ERC20 buy_gem, //maker (ask) buy which token
        uint256 pos //position to insert offer, 0 should be used if unknown
    ) external payable returns (uint256) {
        require(
            msg.value >= pay_amt,
            "didnt send enough native ETH for WETH offer"
        );
        uint256 _before = ERC20(buy_gem).balanceOf(address(this));
        WETH9(wethAddress).deposit{value: pay_amt}();
        uint256 id = RubiconMarket(RubiconMarketAddress).offer(
            pay_amt,
            ERC20(wethAddress),
            buy_amt,
            buy_gem,
            pos
        );
        uint256 _after = ERC20(buy_gem).balanceOf(address(this));
        if (_after > _before) {
            //return any potential fill amount on the offer
            ERC20(buy_gem).transfer(msg.sender, _after - _before);
        }
        return id;
    }

RubiconRouter#depositWithETH

    // Deposit native ETH -> WETH pool
    function depositWithETH(uint256 amount, address targetPool)
        external
        payable
        returns (uint256 newShares)
    {
        IERC20 target = IBathToken(targetPool).underlyingToken();
        require(target == ERC20(wethAddress), "target pool not weth pool");
        require(msg.value >= amount, "didnt send enough eth");

        if (target.allowance(address(this), targetPool) == 0) {
            target.approve(targetPool, amount);
        }

        WETH9(wethAddress).deposit{value: amount}();
        newShares = IBathToken(targetPool).deposit(amount);
        //Send back bathTokens to sender
        ERC20(targetPool).transfer(msg.sender, newShares);
    }

RubiconRouter#swapWithETH

    function swapWithETH(
        uint256 pay_amt,
        uint256 buy_amt_min,
        address[] calldata route, // First address is what is being payed, Last address is what is being bought
        uint256 expectedMarketFeeBPS
    ) external payable returns (uint256) {
        require(route[0] == wethAddress, "Initial value in path not WETH");
        uint256 amtWithFee = pay_amt.add(
            pay_amt.mul(expectedMarketFeeBPS).div(10000)
        );
        require(
            msg.value >= amtWithFee,
            "must send enough native ETH to pay as weth and account for fee"
        );
        WETH9(wethAddress).deposit{value: amtWithFee}();
        return
            _swap(
                pay_amt,
                buy_amt_min,
                route,
                expectedMarketFeeBPS,
                msg.sender
            );
    }

Recommendation: Since expected payment amounts can be calculated exactly by the caller, require exact payment amounts for these functions.

Judge has assessed an item in Issue #435 as Medium risk. The relevant finding follows:

Low

BathToken domain separator is fixed

The BathToken#DOMAIN_SEPARATOR used for EIP-2612 approvals is set permanently in the contract initializer:

BathToken#initializer

        uint256 chainId;
        assembly {
            chainId := chainid()
        }
        DOMAIN_SEPARATOR = keccak256(
            abi.encode(
                keccak256(
                    "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
                ),
                keccak256(bytes(name)),
                keccak256(bytes("1")),
                chainId,
                address(this)
            )
        );

Since the domain separator includes the chainId, there is a risk of permit replay attacks between chains in the event of a future chain split. (See "Security Considerations" in the EIP-2612 spec).

Recommendation: store both CHAIN_ID and DOMAIN_SEPARATOR at contract initialization time. Read the current chainId in permit and recalculate the domain separator if it does not match the cached value.

BathToken admin can set feeBPS to 100%

The BathToken admin can set feeBPS to 100%, which would claim all withdrawals as fees. Additionally, a malicious admin could observe and frontrun withdrawal transactions to increase the fee value and claim additional fees.

BathToken#setFeeBPS

    /// @notice Admin-only function to set a Bath Token's feeBPS
    function setFeeBPS(uint256 _feeBPS) external onlyBathHouse {
        feeBPS = _feeBPS;
    }

Recommendation: Set and validate an upper bound on fees. Ensure the admin account is controlled by a timelock with a reasonable delay for parameter changes to mitigate frontrunning risk.

BathHouse admin can be transferred to the zero address

The BathHouse admin can be intentionally or accidentally set to address(0), which would permanently deny access to onlyAdmin protected functions.

BathHouse#setBathHouseAdmin

    /// @notice Admin-only function to set a new Admin
    function setBathHouseAdmin(address newAdmin) external onlyAdmin {
        admin = newAdmin;
    }

Suggestion: Validate that newAdmin is not address(0) in setBathHouseAdmin:

    /// @notice Admin-only function to set a new Admin
    function setBathHouseAdmin(address newAdmin) external onlyAdmin {
        require(newAdmin != address(0), 'Invalid admin');
        admin = newAdmin;
    }

Additionally, consider implementing two-step ownership transfers, which are a more robust method to prevent accidental transfers.

Prefer two-step admin transfers

It the BathHouse admin accidentally transfers ownership to an incorrect address, protected functions may become permanently inaccessible.

BathHouse#setBathHouseAdmin

    /// @notice Admin-only function to set a new Admin
    function setBathHouseAdmin(address newAdmin) external onlyAdmin {
        admin = newAdmin;
    }

Suggestion: handle admin changes with two steps and two transactions. First, allow the current admin to propose a new owner address. Second, allow the proposed admin (and only the proposed admin) to accept ownership, and update the contract owner internally.

Noncritical

Emit events from permissioned functions

Consider adding events to protected functions that change contract state. This enables you to monitor off chain for suspicious activity, and allows end users to observe and trust changes to these parameters.

-BathHouse#createBathToken -BathHouse#adminWriteBathToken -BathHouse#setBathHouseAdmin -BathHouse#setNewBathTokenImplementation -BathHouse#setPermissionedStrategists -BathHouse#setCancelTimeDelay -BathHouse#setReserveRatio -BathHouse#setMarket -BathToken#setMarket -BathToken#setBathHouse -BathToken#setFeeBPS -BathToken#setFeeTo -BathToken#setBonusToken

QA

Remove unused implicit return value from getExpectedSwapFill

The implicit fill_amt return value in RubiconRouter#getExpectedSwapFill is unused. Instead, the function uses an explicit return on line 188.

RubiconRouter#getExpectedSwapFill

    /// @dev this function takes the same parameters of swap and returns the expected amount
    function getExpectedSwapFill(
        uint256 pay_amt,
        uint256 buy_amt_min,
        address[] calldata route, // First address is what is being payed, Last address is what is being bought
        uint256 expectedMarketFeeBPS //20
    ) public view returns (uint256 fill_amt) {
        address _market = RubiconMarketAddress;
        uint256 currentAmount = 0;
        for (uint256 i = 0; i < route.length - 1; i++) {
            (address input, address output) = (route[i], route[i + 1]);
            uint256 _pay = i == 0
                ? pay_amt
                : (
                    currentAmount.sub(
                        currentAmount.mul(expectedMarketFeeBPS).div(10000)
                    )
                );
            uint256 wouldBeFillAmount = RubiconMarket(_market).getBuyAmount(
                ERC20(output),
                ERC20(input),
                _pay
            );
            currentAmount = wouldBeFillAmount;
        }
        require(currentAmount >= buy_amt_min, "didnt clear buy_amt_min");

        // Return the wouldbe resulting swap amount
        return (currentAmount);
    }

Simplify boolean checks in isApprovedStrategist

The logic in BathHouse#isApprovedStrategist can be simplified by omitting a boolean equality check and directly returning the value.

BathHouse.sol#372

    /// @notice A function to check whether or not an address is an approved strategist
    function isApprovedStrategist(address wouldBeStrategist)
        external
        view
        returns (bool)
    {
        if (
            approvedStrategists[wouldBeStrategist] == true ||
            !permissionedStrategists
        ) {
            return true;
        } else {
            return false;
        }
    }

Suggestion:

    /// @notice A function to check whether or not an address is an approved strategist
    function isApprovedStrategist(address wouldBeStrategist)
        external
        view
        returns (bool)
    {
        return (approvedStrategists[wouldBeStrategist] || !permissionedStrategists);
    }

Set initialized at top of initializers

In the initialize functions for both BathHouse, and BathToken, initialized is set to true at the very end of the function. In the case of BathToken, this value is set after making an external call to set a token approval. Consider setting initialized at the start of the initializer function, which is more consistent with checks-effects-interactions and a good defense in depth habit against potential re-entrancy.

Incorrect natspec comments

BathHouse#setBathTokenMarket`(https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L285-L291)

    /// @notice Admin-only function to set a Bath Token's timeDelay
    function setBathTokenMarket(address bathToken, address newMarket)
        external
        onlyAdmin
    {
        IBathToken(bathToken).setMarket(newMarket);
    }

Lowercase RubiconMarketAddress

Consider using a lowercase name for the RubiconMarketAddress address, which is consistent with the Solidity style guide.

RubiconRouter..sol#19

    address public RubiconMarketAddress;