QA Report

[code423n4]

Here are 11 QA findings per file.

---

BathToken.sol

[QA-1] Naming inconsistency - consider using camel case

Following codes use pool_asset although other IERC20 variable (e.g., transferAsset) is named with camel case.

IERC20 pool_asset,

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L123
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L136
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L146
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L154

They should be following:

IERC20 poolAsset,

---

[QA-2] Unnecessary parentheses

return (id);

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

Parentheses wrapping id is not necessary, and can be written like this:

return id;

---

[QA-3] Inconsistent return variables

There are some functions which have return variable is defined but return is used in the function.

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

    function withdraw(uint256 _shares)
        external
        returns (uint256 amountWithdrawn)
    {
        return _withdraw(_shares, msg.sender);
    }

On the other hand, other functions set return variable and does not use return at the function.

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

    function asset() public view returns (address assetTokenAddress) {
        assetTokenAddress = address(underlyingToken);
    }

It depends on the dev's preference, but for the consistency, withdraw function can be written like this if it defines amountWithdrawn return variable:

    function withdraw(uint256 _shares)
        external
        returns (uint256 amountWithdrawn)
    {
        amountWithdrawn = _withdraw(_shares, msg.sender);
    }

Aside from withdraw function, following 4 functions define return variable although the defined variables are not used.

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L388-L390
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L435-L438
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L441-L446
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L525-L527

---

BathPair.sol

[QA-4] Add handling when current.length becomes 0 at handleStratOrderAtID function

handleStratOrderAtID function has following logic:

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

        uint256[] storage current = outOffersByStrategist[_asset][_quote][
            info.strategist
        ];
        current[target] = current[current.length - 1];

When current is empty uint256 array, current.length - 1 will be underflown. As a result, current[target] = current[current.length - 1] would fail since current[underflown index] may not have a proper value.

Depending on the expected product behavior, this part should have more additional checks to prevent the errors.

---

[QA-5] Naming inconsistency - consider using camel case

Although function names and variable names use camel case consistently, but following two function and variable are the exceptions.

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

uint256 internal last_stratTrade_id;

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

function _next_id() internal returns (uint256) {

It should consider using the camel case to be consistent.

---

BathHouse.sol

[QA-6] Unnecessary parentheses

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

address pairedPool = getBathTokenfromAsset((desiredPairedAsset));

There is an extra parentheses wrapping desiredPairedAsset. It can be written like this:

address pairedPool = getBathTokenfromAsset(desiredPairedAsset);

---

[QA-7] Incorrect comment on setBathTokenMarket function

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

    /// @notice Admin-only function to set a Bath Token's timeDelay
    function setBathTokenMarket(address bathToken, address newMarket)

setBathTokenMarket function seems not setting timeDelay.

---

[QA-8] Extra space and line exists

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

            TransparentUpgradeableProxy newBathToken
         = new TransparentUpgradeableProxy(
            newBathTokenImplementation,
            proxyManager,
            _initData
        );

The above part can be written like this by removing extra line and space.

        TransparentUpgradeableProxy newBathToken
        = new TransparentUpgradeableProxy(
            newBathTokenImplementation,
            proxyManager,
            _initData
        );

---

RubiconRouter.sol

[QA-9] Unnecessary parentheses

Following part has unnecesssary parentheses.

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L173-L177

                : (
                    currentAmount.sub(
                        currentAmount.mul(expectedMarketFeeBPS).div(10000)
                    )
                );

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L188

return (currentAmount);

They can be written like these by removing unnecessary parentheses:

                : currentAmount.sub(
                    currentAmount.mul(expectedMarketFeeBPS).div(10000)
                );

return currentAmount;

---

[QA-10] Typo in the comment at various functions

payed seems to be typo in the comment.

First address is what is being payed, Last address is what is being bought

Following three places have the typo.
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L221
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L197
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L164

In this contract, paid seems to be correct.

---

[QA-11] Inconsistent return variable

buyAllAmountForETH function defines file variable at its return value.

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L365

However, return fill is executed at the end of the function.

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L379

Throughout this file, when function defines return variable, it does not actually execute return at the end of the function. (e.g., buyAllAmountWithETH function). So it should be better to follow how buyAllAmountWithETH or other functions do.

---