QA Report

[code423n4]

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;