QA Report

[code423n4]

QA (LOW & NON-CRITICAL)

• transfer and safeTransfer methods are used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable, it may likely to get broken when calling a contract's fallback function.
  Reference Link -1, Reference Link -2

• Floating Pragma used in VestingWallet.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
  Reference

• The whole project have different solidity compiler ranges ( 0.5.0 - 0.8.0) referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.

• The project mostly uses Solidity version 0.7.6. Using an old version prevents access to new Solidity security checks. However the current version is 0.8.14 with more benefits and less bugs.

• block.timestamp is used on many places at the scoped contracts. However, this can be manipulated by malicious miners like the options can be shown as expired before the end of the auction.
  Reference

• The codebase uses isContract() function which should not be relied on if the target is a contract inside the construction. Reference is here

• The contract uses ecrecover() function. EVM's ecrecover is susceptible to signature malleability which allows replay attacks, but using OpenZeppelin’s ECDSA library can be mitigation in BathToken.sol for permit() function. Reference

• The team can consider to state the infinite allowance by using type(uint256).max instead of uint256(-1) which is easier to read.

• There is no address(0) or Zero value check at:
  BathHouse.sol; initialize() function for the params: market, _newBathTokenImplementation, _proxyAdmin
  initBathPair() function for the params: _bathPairAddress ,

• Critical address changes are not done in two steps for the followings:
  In BathHouse.sol for setBathHouseAdmin(), setBathHouseAdmin()
  In BathToken.sol for setMarket(), setBathHouse(),
  In RubiconMarket.sol for setOwner(),
  Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e. claims ownership). This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible. Reference, Reference

• When changing state variables events are not emitted for some functions:
  In BathToken.sol for setBathTokenMarket(), setBonusToken() or for Admin Only functions where the critical address changes like BathToken.sol's setBathHouse()

• The OZ ERC20.sol imported 2 times at here

• The require statements do not throw error messages for the following functions:
  For BathHouse.sol; initialize() function , setReserveRatio() function
  When a require statement did not pass, it will not be possible to know which require statement did not pass since it does not throw error.

• At RubiconMarket.sol, can_offer() modifier is confusing since there is no implementation function and there is no NatSpec for the modifier's service.

• At RubiconMarket.sol, the offer() function, parameters are casted to uint128 without safe math. Not using SafeMath could lead to under-/overflows in certain cases.It also makes it hard to reason about the code, as the reverts only happen at a place that is far from the original overflow/unsafe cast. Updating to a newer Solidity version will break a lot of the code.
  Recommendation: Use SafeMath by default.Even if it seems like it's not needed in some places, it could still be that there is some code path that leads to issues that one didn't think of, or gets implemented in the future.

• At RubiconMarket.sol, pay_gem was not approved to be transferred at require(pay_gem.transferFrom(msg.sender, address(this), pay_amt));

• At RubiconRouter.sol's swap() function there is no zero value check for the params:pay_amt & buy_amt_min

• At VestingWallet.sol, OpenZeppelin's Address.sol is imported along with the SafeERC20.sol. However, SafeERC20.sol already inherits Address.sol. This created multiple inheritance. Contracts inheriting from multiple contracts with identical functions should specify the correct inheritance order i.e. more general to more specific to avoid inheriting the incorrect function implementation. Reference

• At VestingWallet.sol, the imported contracts are SafeERC20.sol, Address.sol, Context.sol, Math.sol. However, no proper contract identifier shown, leaving the contract not inheriting or using X library for Y variable.

• At VestingWallet.sol, variables _start,_duration, _released are implicitly converted without using Safe math or OZ's SafeCast Library. This may result in overflow/underflow especially in release() function.

    function start() public view virtual returns (uint256) {
        return _start;

    function duration() public view virtual returns (uint256) { 
        return _duration; 
    }

    function release() public virtual {
        uint256 releasable = vestedAmount(uint64(block.timestamp)) - released(); 
        _released += releasable;
        emit EtherReleased(releasable);
        Address.sendValue(payable(beneficiary()), releasable);
    }

Also at _vestingSchedule() the start() function calls the result variable as casted uint256 without safecast/safe math. This may cause underflow/overflow and result in timestamp < start() all the time and returns 0.

• At VestingWallet.sol's release() function, the team can consider to emit the event after successfull sendValue method.

• At BathHouse.sol's adminWriteBathToken() function, there should be some require statement or mitigation for the advantage of the users. The users who are holding ex-bathtoken should not be affected by this call.

• At BathHouse.sol's setCancelTimeDelay() function, in order to make the time delay variable meaningful, there should be upper limits for the function. Say the delay is set arbitrarily to a very unlikely high value, the users will certainly have the disadvantage of this.

• At BathHouse.sol's _createBathToken() function, there is no address(0) check for _feeAdmin parameter, it may risk the fees to send to address(0).

• At BathHouse.sol's _createBathToken() function, there is no address(0) check for newBathTokenAddress = address(newBathToken); at https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L432

• BathPair.sol's initialize function can be frontrun since require(msg.sender == address(BathHouse)) check is not one during initializing when called from this line in BathHouse.sol .

• At BathToken.sol's setFeeTo() function there is no address(0) check

• At BathToken.sol's setBonusToken() function, the parameter newBonusERC20 is not verified against confirming ERC20 complaint.

• At BathToken.sol's withdraw() function, there is a check for assetsReceived >= assets. However, this might not work for deflationary tokens. Or the logic should be assetsReceived <= assets

• At BathToken.sol's mint() function, if an attacker send 1 wei to the underlying token vault (or the token is deflationary), the require statement will return false and the assets will be locked at the vault.

    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"); //@audit 
    }

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

[HickupHH3]

At BathHouse.sol's adminWriteBathToken() function, there should be some require statement or mitigation for the advantage of the users. The users who are holding ex-bathtoken should not be affected by this call.

At BathToken.sol's setBonusToken() function, the parameter newBonusERC20 is not verified against confirming ERC20 complaint.

could have used a bit more elaboration to qualify as a rug pull vector