Rubicon contest - hubble's results

Lines of code

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

Vulnerability details

rewardsVestingWallet (IBathBuddy) helps in releasing or distributing vested bonus tokens during withdrawals. There is currently no function to set it in BathToken contract. rewardsVestingWallet (IBathBuddy) remains as address(0) and is unusable.

Impact

Admin can not add BathBuddy. User can not get bonus token incentives.

Proof of Concept

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

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

Tools Used

Manual review

Recommended Mitigation Steps

Add the following function to BathToken contract

/// @notice Admin-only function to set a Bath Token's rewardsVestingWallet
function setRewardsVestingWallet(address rewardsVestingWallet_) external onlyBathHouse {
    require(rewardsVestingWallet_ != address(0), "invalid_addr");
    rewardsVestingWallet = rewardsVestingWallet_;
}

Accordingly, add a new function to BathHouse also.

END

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L270 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L629 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L98-L101

Vulnerability details

The function setBonusToken allows the same BonusToken to be added more than once to the array bonusTokens.

  function setBonusToken(address newBonusERC20) external onlyBathHouse {
    bonusTokens.push(newBonusERC20);
  }

Impact

If that happens, early withdrawers can get Bonus in multiples of what they actually have right to. Late withdrawers, might not get any Bonus due to shortage.

Proof of Concept

BathToken.sol, function setBonusToken https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L270-L272

1. function setBonusToken allows the same BonusToken to be added more than once to the array.

BathToken.sol, function distributeBonusTokenRewards https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L629 2. a. As and when distributeBonusTokenRewards is triggered during a withdraw call, the same bonusToken will be released more than once.

BathBuddy.sol, function release https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L98-L101 2. b. The release function is called.

Tools Used

Manual review

Recommended Mitigation Steps

Add the required validations to avoid duplicate additions of bonus tokens.

  function setBonusToken(address newBonusERC20) external onlyBathHouse {
    require(newBonusERC20 != address(0), "invalid_addr");
    if (bonusTokens.length > 0) {
      for (uint256 index = 0; index < bonusTokens.length; index++) {
        require (token != newBonusERC20, "token already exists")
      }
    }
    bonusTokens.push(newBonusERC20);
  }

Lines of code

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

Vulnerability details

Contract BathHouse.sol

function setCancelTimeDelay(uint256 value) function setReserveRatio(uint256 rr) function setBathTokenFeeBPS(address bathToken, uint256 newBPS) function setBathTokenFeeTo(address bathToken, address feeTo)

Contract BathToken.sol

function setFeeBPS(uint256 _feeBPS)

These functions that make critical changes should have a timelock, so that users can see the upcoming changes and have time to react to these changes. Another thing to note is that there is no input validation like lower/upper bound check, address check, etc., currently in these functions.

Impact

Stable and trustworthy protocol.

Proof of Concept

https://consensys.net/diligence/audits/2020/12/1inch-liquidity-protocol/#unpredictable-behavior-for-users-due-to-admin-front-running-or-general-bad-timing https://github.com/code-423n4/2022-01-sandclock-findings/issues/178

Recommended Mitigation Steps

Add timelock for critical changes which will effect user experience and decisions.

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

Summary of Findings for Low / Non-Critical issues

LOW

• L-01 : Unused function parameter _feeTo (BathToken.sol, function initialize)
• L-02 : Missing basic validation for parameter _feeBPS (BathToken.sol, function setFeeBPS)
• L-03 : reserveRatio can be set very low like 1
• L-04 : the feeTo should always be the contract address

NON-CRITICAL

• NC-01 : Duplicate IWETH interface (RubiconMarket.sol)
• NC-02 : Function name withdraw(uint256 _shares) is confusing with EIP-4626 standard function withdraw(uint256 assets, address receiver, address owner) (BathToken.sol)
• NC-03 : Custom defined TransparentUpgradeableProxy contract not used anywhere (BathHouse.sol)

Details L-01

Title : Unused function parameter _feeTo (BathToken.sol, function initialize)

Impact

Proof of Concept

Contract : BathToken.sol Function : initialize

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

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

Recommended Mitigation Steps

Either,

• Remove the parameter _feeTo. Or,
• At line 217, update feeTo to be set as _feeTo when it is not address(0)

Details L-02

Title : Missing basic validation for parameter _feeBPS (BathToken.sol, function setFeeBPS)

Impact

Proof of Concept

Contract : BathToken.sol Function : setFeeBPS

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

Recommended Mitigation Steps

Add the following validation, require(_feeBPS <= 5, "")

Details L-03

Title : reserveRatio can be set very low like 1

As per the whitepaper, to protect Rubicon Pools LPs and for security of the protocol, some amount of pool liquidity should be present in the contract at all times. So, allowing to set the ReserveRation to very close to 0 should be disallowed. A check for a min value like MIN_RR = 20 is recommened to be added.

Impact

reserveRatio can be set to 1, which may drain the liquidity from the pool.

Proof of Concept

Contract : BathHouse.sol Function : setReserveRatio()

Recommended Mitigation Steps

define a constant and add a require statement in setReserveRatio() like below

MIN_ReserveRatio = 20 require(rr > 0);

Details L-04

Title : the feeTo should always be the contract address

As per the whitepaper, related to FeeTo destination address. "These fees go to the pool, so it is set to the BathToken contract address for a given pool." But there is no check in the setFeeTo() function if the _feeTo address is contract pool address.

Impact

Malicious or by error, the FeeTo can be changed to any external address, so LP's will be losing the rewards.

Proof of Concept

Contract : BathToken.sol

function setFeeTo(address _feeTo) external onlyBathHouse {
        feeTo = _feeTo;
    }

Recommended Mitigation Steps

Once the feeTo is set during initialization, it should not be configurable.

Details NC-01

Title : Duplicate IWETH interface (RubiconMarket.sol)

Impact

Compiler warning

Proof of Concept

Contract : RubiconMarket.sol https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L1262-L1270

Recommended Mitigation Steps

Remove from interface definition from here, and import from interfaces/IWETH.sol

Details NC-02

Title : Function name withdraw(uint256 _shares) is confusing with EIP-4626 standard function withdraw(uint256 assets, address receiver, address owner) (BathToken.sol)

Impact

Confusion EIP-4626 standard function withdraw(uint256 assets, address receiver, address owner) expects assets as its first input parameter Function name withdraw(uint256 _shares) expects shares as input parameter

Proof of Concept

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

Recommended Mitigation Steps

Rename the function withdraw(uint256 _shares) to

• withdrawByShares(uint256 _shares) or
• burn(uint256 _shares)

Details NC-03

Title : Custom defined TransparentUpgradeableProxy contract not used anywhere (BathHouse.sol)

Custom defined TransparentUpgradeableProxy contract is not used anywhere, instead OZ's TransparentUpgradeableProxy is used in BathHouse.sol

Proof of Concept

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