QA Report

[code423n4]

L01 - Missing zero address checks in admin setters

Some functions missing zero address checks when setting admin addresses, which could lead to loss of admin control.

Apply a zero-address check and consider implementing a two-step process transferOwnership, where the owner assigns an account and the designated account must call the acceptOwnership() function for full transfer of ownership.

    function setOwner(address owner_) external auth { 
        owner = owner_;
        emit LogSetOwner(owner);
    }

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L22-L25

    function setBathHouseAdmin(address newAdmin) external onlyAdmin { 
        admin = newAdmin;
    }

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L253-L255

    function setBathHouse(address newBathHouse) external onlyBathHouse {  
        bathHouse = newBathHouse;
    }

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L250-L252

L02 - Missing zero address checks in _deposit() and _withdraw() functions

User could mistakenly set receiver parameter to zero address in deposit(), mint(), withdraw() and redeem() functions. This would lead to loss of user funds.
Recommend to add check for zero address for receiver parameter.

L03 - ERC20 tokens with no return value will fail to transfer in functions buy(), cancel(), offer(), swapForETH(), openBathTokenSpawnAndSignal()

Although the ERC20 standard suggests that a transfer should return true on success, many tokens are non-compliant in this regard. In that case, the call here will revert even if the transfer is successful:

        require(
            _offer.buy_gem.transferFrom(msg.sender, feeTo, fee), 
            "Insufficient funds to cover fee"
        );
        
        ...
        
        require(
            _offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend), 
            "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee"  
        );
        require(
            _offer.pay_gem.transfer(msg.sender, quantity), 
            "_offer.pay_gem.transfer(msg.sender, quantity) failed" 
        );

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L272-L347

        require(_offer.pay_gem.transfer(_offer.owner, _offer.pay_amt));

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L351-L376

        require(pay_gem.transferFrom(msg.sender, address(this), pay_amt));

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L392-L429

        require(
            ERC20(route[0]).transferFrom( 
                msg.sender,
                address(this),
                pay_amt.add(pay_amt.mul(expectedMarketFeeBPS).div(10000)) 
            ),
            "initial ERC20 transfer failed"
        );

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L519-L549

        require(
            newBathTokenUnderlying.transferFrom( 
                msg.sender,
                address(this),
                initialLiquidityNew
            ),
            "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
        );

        ...

        require(
            desiredPairedAsset.transferFrom(
                msg.sender,
                address(this),
                initialLiquidityExistingBathToken
            ),
            "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol" 
        );

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L136-L203

L04 - Lack of event emitting after sensitive actions

Contracts do not emit relevant events after setting sensitive variables.

Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity in following functions:

    function setFeeBPS(uint256 _newFeeBPS) external auth returns (bool) { 
        feeBPS = _newFeeBPS;
        return true;
    }

    ...

    function setFeeTo(address newFeeTo) external auth returns (bool) {
        feeTo = newFeeTo;
        return true;
    }

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L1231-1256

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

    /// @notice Admin-only function to set a new Bath Token implementation
    /// @dev Please note that all bathTokens created will use this abi
    function setNewBathTokenImplementation(address newImplementation) external onlyAdmin {
        newBathTokenImplementation = newImplementation;
    }

    /// @notice Admin-only function to approve a new permissioned strategist
    function approveStrategist(address strategist) public onlyAdmin {
        approvedStrategists[strategist] = true;
    }

    /// @notice Admin-only function to set whether or not strategists are permissioned
    function setPermissionedStrategists(bool _new) external onlyAdmin {
        permissionedStrategists = _new;
    }

    /// @notice Admin-only function to set timeDelay
    function setCancelTimeDelay(uint256 value) external onlyAdmin {
        timeDelay = value;
    }

    /// @notice Admin-only function to set reserveRatio
    function setReserveRatio(uint256 rr) external onlyAdmin {
        require(rr <= 100); 
          require(rr > 0);
        reserveRatio = rr;
    }

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L253-L283

    /// @notice Admin-only function to set a Bath Token's market address
    function setMarket(address newRubiconMarket) external onlyBathHouse { 
        RubiconMarketAddress = newRubiconMarket; 
    }

    /// @notice Admin-only function to set a Bath Token's Bath House admin
    function setBathHouse(address newBathHouse) external onlyBathHouse { 
        bathHouse = newBathHouse;
    }

    /// @notice Admin-only function to approve Bath Token's RubiconMarketAddress with the maximum integer value (infinite approval)
    function approveMarket() external onlyBathHouse {
        underlyingToken.approve(RubiconMarketAddress, 2**256 - 1); 
    }

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

    /// @notice Admin-only function to set a Bath Token's fee recipient, typically the pool itself
    function setFeeTo(address _feeTo) external onlyBathHouse { 
        feeTo = _feeTo;
    }

    /// @notice Admin-only function to add a bonus token to bonusTokens for pool incentives
    function setBonusToken(address newBonusERC20) external onlyBathHouse {
        bonusTokens.push(newBonusERC20);
    }

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L245-L272

L05 - Drain bonus tokens

Since bonus tokens distribute each time users withdraw() their shares, there are possible situations when fee amount for withdrawal is less than value of bonus token distributed. It could lead to draining bonus tokens by malicious users which repeatedly deposit-withdraw their shares unlit it's profitably - leaving other shares holders without bonus tokens.

Consider distribution of bonus tokens based on shares holding time.

L06 - Adding new orders to list of strategist orders could lead to Dos

Function getIndexFromElement() iterates through list of strategist open orders, if list would be too big it could run out of gas.
It could lead to inability to cancel strategist orders on pair, since function handleStratOrderAtID() calling getIndexFromElement().

Consider adding limitation for number of open orders by strategist on one pair.

    function getIndexFromElement(uint256 uid, uint256[] storage array)
        internal
        view
        returns (uint256 _index)
    {
        bool assigned = false;
        for (uint256 index = 0; index < array.length; index++) { 
            if (uid == array[index]) {
                _index = index;
                assigned = true;
                return _index;
            }
        }
        require(assigned, "Didnt Find that element in live list, cannot scrub"); 
    }

L07 - Adding new bonus tokens could lead to Dos

Admin could only add new bonus tokens for withdrawers using function setBonusToken().

Withdraw transaction calling to distributeBonusTokenRewards() which could run out of gas if list of bonus tokens will be too big.
This would lead to the inability of users to withdraw their assets.

Consider adding a function that allows the admin to delete addresses from bonusTokens.

N01 - Variable rewardsVestingWallet never change

Variable rewardsVestingWallet never changes and stays zero value which means that distribution of bonus tokens to pool withdrawers would not be possible
in current implementation.

N02 - mint() function wouldn't work with fee-on-transfer tokens

Function mint() require to shares parameter to be equal to amount of shares that are minted inside _deposit() function, but with FOT tokens minted shares amount would be less than expected, since previewMint() function count assets amount without fee subtraction and transaction would be failed.

    function mint(uint256 shares, address receiver) 
        public
        returns (uint256 assets)
    {
        assets = previewMint(shares);
        uint256 _shares = _deposit(assets, receiver);
        require(_shares == shares, "did not mint expected share count"); 
    }

N03 - Not used function, variable and event

Function released(), event EtherReleased() never used and variable _released never changes, remove it for readability and conciseness.

    event EtherReleased(uint256 amount);

    uint256 private _released;

    /**
     * @dev Amount of eth already released
     */
    function released() public view returns (uint256) {
        return _released;
    }

N04 - Not specified visibility for variable

    bool locked;

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L191

N05 - Commented code

Some lines of code are commented out, remove it for readability and conciseness.

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L93-L110

N06 - Missing revert strings in require statements

Add messages to the next require statements to make contracts more self-explanatory.

contracts/RubiconMarket.sol:210
contracts/RubiconMarket.sol:216
contracts/RubiconMarket.sol:217
contracts/RubiconMarket.sol:226
contracts/RubiconMarket.sol:432
contracts/RubiconMarket.sol:459
contracts/RubiconMarket.sol:460
contracts/RubiconMarket.sol:466
contracts/RubiconMarket.sol:467
contracts/RubiconMarket.sol:702
contracts/RubiconMarket.sol:703
contracts/RubiconMarket.sol:865
contracts/RubiconMarket.sol:906
contracts/RubiconMarket.sol:960
contracts/RubiconMarket.sol:970
contracts/RubiconMarket.sol:985
contracts/RubiconMarket.sol:1002
contracts/RubiconMarket.sol:1119
contracts/RubiconMarket.sol:1131
contracts/RubiconMarket.sol:1175
contracts/RubiconMarket.sol:1180
contracts/RubiconMarket.sol:1184
contracts/RubiconMarket.sol:1193
contracts/RubiconMarket.sol:1209
contracts/RubiconRouter.sol:42
contracts/rubiconPools/BathHouse.sol:88
contracts/rubiconPools/BathHouse.sol:104
contracts/rubiconPools/BathHouse.sol:110
contracts/rubiconPools/BathHouse.sol:111
contracts/rubiconPools/BathHouse.sol:280
contracts/rubiconPools/BathHouse.sol:281
contracts/rubiconPools/BathPair.sol:118
contracts/rubiconPools/BathPair.sol:143
contracts/rubiconPools/BathToken.sol:186

N07 - Extra brackets

        address pairedPool = getBathTokenfromAsset((desiredPairedAsset)); 

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L179

N08 - Typos

contracts/RubiconMarket.sol:851    fill_amt = add(fill_amt, offers[offerId].pay_amt); //Add amount bought to acumulator // typo - accumulator
contracts/RubiconMarket.sol:860    fill_amt = add(fill_amt, baux); //Add amount bought to acumulator // typo - accumulator
contracts/RubiconMarket.sol:890    fill_amt = add(fill_amt, offers[offerId].buy_amt); //Add amount sold to acumulator  // typo - accumulator
contracts/RubiconMarket.sol:901    ); //Add amount sold to acumulator  // typo - accumulator
contracts/RubiconRouter.sol:187 // Return the wouldbe resulting swap amount // typo - would be
contracts/peripheral_contracts/BathBuddy.sol:110    // Keep tokens here by not transfering the _fee anywhere, it is accrued to the Bath Token's Bath Buddy // typo - transferring
contracts/rubiconPools/BathHouse.sol:132    /// @notice Please note, creating a Bath Token in this fashion ~does not~ gaurentee markets will be made for the new pair. This function signals the desire to have a new pair supported on Rubicon for strategists to consider market-making for // typo - guarantee
contracts/rubiconPools/BathHouse.sol:414    // Note, the option of a fee recipient for pool withdrawls exists. For all pools this is set to the pool itself in production and is visible via ~feeTo~ on any respective contract // typo - withdrawals
contracts/rubiconPools/BathPair.sol:211    /// @dev The local array of strategist IDs that exists for any given strategist [query via getOutstandingStrategistTrades()] acts as an acitve RAM for outstanding strategist trades // typo - active
contracts/rubiconPools/BathToken.sol:29    /// @notice The RubiconMarket.sol instance that all pool liquidity is intially directed to as market-making offers // typo - initially
contracts/rubiconPools/BathToken.sol:627    /// @notice Function to distibute non-underlyingToken Bath Token incentives to pool withdrawers // typo - distribute
contracts/rubiconPools/BathHouse.sol:355    function getBathTokenfromAsset(ERC20 asset) public view returns (address) { // Should be capital `F` in function name