Rubicon contest - pauliax's results

Lines of code

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

Vulnerability details

Impact

rewardsVestingWallet in BathToken is never initialized thus release will never happen:

    /// @notice Address of the OZ Vesting Wallet which acts as means to vest bonusToken incentives to pool HODLers
    IBathBuddy public rewardsVestingWallet;

When calling distributeBonusTokenRewards this will be skipped:

  if (rewardsVestingWallet != IBathBuddy(0)) {
      rewardsVestingWallet.release(
          (token),
          receiver,
          sharesWithdrawn,
          initialTotalSupply,
          feeBPS
      );
  }

Recommended Mitigation Steps

Consider either initializing rewardsVestingWallet or restructuring this functionality.

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

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L69

Vulnerability details

Impact

BathBuddy has an empty receive function but there is no function to get the native asset out, thus it will be stuck in the contract forever. Release function for native token was present in VestingWallet but removed in BathBuddy: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.6/contracts/finance/VestingWallet.sol#L85-L89

Recommended Mitigation Steps

Consider removing this function to prevent accidental loss of a native asset.

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

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

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

Lines of code

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

Vulnerability details

Impact

RubiconMarketAddress in BathPair is initialized only once:

  RubiconMarketAddress = IBathHouse(_bathHouse).getMarket();

but market can change in Bath house:

    /// @notice Admin-only function to set a Bath Token's target Rubicon Market
    function setMarket(address newMarket) external onlyAdmin {
        RubiconMarketAddress = newMarket;
    }

Thus it will get out of sync. Also, the comment says that it changes Bath Token's target Rubicon market but actually it updates its own instance variable.

Recommended Mitigation Steps

I think RubiconMarketAddress should sync between BathPair and BathHouse.

• SafeMath library is only effective with uint256 types:

  using SafeMath for uint16;

Also, uint16 does not make any sense, nor give any improvements here:

  uint16 stratReward = IBathHouse(_bathHouse).getBPSToStrats();
  function getBPSToStrats() external view returns (uint8);

Consider storing stratReward in uint256 and removing using SafeMath for uint16.

• timeDelay and all the functions related to it are not used in any meaningful way. Depending on the intentions, consider either removing it or implementing it where intended.

   /// @notice A variable time delay after which a strategist must return funds to the Bath Token
    uint256 public timeDelay;

• BathHouse imports the same contract twice:

    import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
    import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

• Misleading comment:

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

• From the whitepaper: "BathTokens can be redeemed for the underlying tokens at any time". However, there exists a reserve ratio, that is initially set to 50%, meaning not all the liquidity is available to redeem at any time. Please keep your users informed and aware of this.

• From the whitepaper: FeeBPS "The fee is currently set to zero but could change in the near future"

  feeBPS = 3; //Fee set to 3 BPS initially

• No need for assembly here, can get it from block.chainid:

  assembly {
    chainId := chainid()
  }

• The current best practice is to use safe ERC20 library for token interactions (safeApprove and safeTransfer), e.g.:

   newBathTokenUnderlying.approve(newOne, initialLiquidityNew);
   IERC20(asset).transfer(msg.sender, booty);

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

• Consider using SafeCast library:

  require(uint128(pay_amt) == pay_amt);
  require(uint128(buy_amt) == buy_amt);

Also, there are many unchecked casts, e.g.:

  take(bytes32(offerId), uint128(offers[offerId].pay_amt)); //We take the whole offer
  take(bytes32(offerId), uint128(baux)); //We take the portion of the offer that we need

Make sure that large values will not be truncated due to explicit casting and utilize safe casts where possible.

• The array of bonusTokens only grows, elements can't be removed. Consider introducing a reasonable upper limit or remove function, otherwise it may grow so large to that calls to distributeBonusTokenRewards will start reverting due to block gas limitations.

    /// @notice Array of Bonus ERC-20 tokens that are given as liquidity incentives to pool withdrawers
    address[] public bonusTokens;

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

    for (uint256 index = 0; index < bonusTokens.length; index++)

• Lack of re-entrancy protection, e.g. function strategistBootyClaim first transfers tokens and only then updates the state. Tokens may contain transfer hooks that can be used to exploit re-entrancy. Either make sure you 100% trust the callers (usually strategists) or implement a re-entrancy guard.

• In tailOff a strategist can choose any _stratUtil target. A malicious target can drain the tokens. Consider having a whitelist, at least temporary, and you can later disable it if everything goes smoothly.

• Contracts use Solidity version that does not protect from overflow / underflow by default, however, there are regular math operations that could be exploited, e.g.:

        uint256 askDelta = info.askPayAmt - offer1.pay_amt;
        uint256 bidDelta = info.bidPayAmt - offer2.pay_amt;

Or:

        uint256 releasable = vestedAmount(
        address(token),
        uint64(block.timestamp)
    ) - released(address(token));
    _erc20Released[address(token)] += amount;

Consider using SafeMath operations everywhere.

• Not used anywhere:

  uint256 private _released;
  event EtherReleased(uint256 amount);

• static constant name could be initially set in a constant variable:

  name = "Rubicon Bath House";

• In openBathTokenSpawnAndSignal address newOne = _createBathToken... can be replaced with newBathToken = _createBathToken...

• Would be a bit cheapier to just directly use scrubStrategistTrade(ids[index]) here:

  uint256 _id = ids[index];
  scrubStrategistTrade(_id);

• If assign is set to true it will always return afterwards without hiting the last line:

  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");

Thus it would be cheapier to replace this code snippet with something like this:

  for (uint256 index = 0; index < array.length; index++) {
      if (uid == array[index]) {
          return index;
      }
  }
  revert("Didnt Find that element in live list, cannot scrub");

• In placeMarketMakingTrades this will never be true, because if either of these addresses are empty, then underlyingBalance calls will fail:

  require(
    bathAssetAddress != address(0) && bathQuoteAddress != address(0),
    "tokenToBathToken error"
  );

  IBathToken(bathAssetAddress).underlyingBalance()
  IBathToken(bathQuoteAddress).underlyingBalance()

• No need to check if token exists here, because it will be later checked in _createBathToken:

  function openBathTokenSpawnAndSignal
    require(
    getBathTokenfromAsset(newBathTokenUnderlying) == address(0),
    "bathToken already exists for that ERC20"
  );

_createBathToken:

  // Check that it isn't already logged in the registry
  require(
      tokenToBathToken[_underlyingERC20] == address(0),
      "bathToken already exists"
  );

• function batchMarketMakingTrades has onlyApprovedStrategist(msg.sender) modifier, but this modifier is also applied every time in placeMarketMakingTrades. Consider extracting common functionality into a separate internal function to avoid useless repeated checks. Similarly, with requote and batchRequoteOffers. Also, scrubStrategistTrade and scrubStrategistTrades.

• Results of repeated external calls should be cached, e.g.: IBathHouse(bathHouse).reserveRatio() called twice:

        require(
            (
                IBathToken(bathAssetAddress).underlyingBalance().mul(
                    IBathHouse(bathHouse).reserveRatio()
                )
            ).div(100) <= IERC20(underlyingAsset).balanceOf(bathAssetAddress),
            "Failed to meet asset pool reserve ratio"
        );
        require(
            (
                IBathToken(bathQuoteAddress).underlyingBalance().mul(
                    IBathHouse(bathHouse).reserveRatio()
                )
            ).div(100) <= IERC20(underlyingQuote).balanceOf(bathQuoteAddress),
            "Failed to meet quote pool reserve ratio"
        );

IBathHouse(_bathHouse).getMarket() called twice:

  require(
      IBathHouse(_bathHouse).getMarket() !=
          address(0x0000000000000000000000000000000000000000) &&
          IBathHouse(_bathHouse).initialized(),
      "BathHouse not initialized"
  );
  bathHouse = _bathHouse;

  RubiconMarketAddress = IBathHouse(_bathHouse).getMarket();

• Repeated storage access should be cached, e.g.: outOffersByStrategist[_asset][_quote][info.strategist] accessed twice:

  uint256 target = getIndexFromElement(
    id,
    outOffersByStrategist[_asset][_quote][info.strategist]
  );
  uint256[] storage current = outOffersByStrategist[_asset][_quote][
    info.strategist
  ];

• Repeated casting should be cached, e.g.: uint128(pay_amt) and uint128(buy_amt) casted twice:

  require(uint128(pay_amt) == pay_amt);
  require(uint128(buy_amt) == buy_amt);
  
  emit LogMake(
    bytes32(id),
    keccak256(abi.encodePacked(pay_gem, buy_gem)),
    msg.sender,
    pay_gem,
    buy_gem,
    uint128(pay_amt),
    uint128(buy_amt),
    uint64(block.timestamp)
  );