QA Report

[code423n4]

Low Risk Vulnerabilities

1. Missing checks in adminWriteBathToken

Rewriting tokenToBathToken mapping without checks could potentially cause some disruption:

• When there are outstanding orders, rewriting bathToken address will prevent strategists from cancelling the orders as tokenToBathToken now points to other contract.
• If admin mistakenly submits newBathToken which has a different underlying token as overwriteERC20, strategist operations might cause irregularity.

Recommended Mitigation

• Add a check to ensure that old bathToken has no outstanding orders before rewriting it. Consider adding a function in BathHouse to cancel all outstanding orders of a bathToken.
• Add a check to ensure the new bathToken's underlying token is the same as overwriteERC20.

  require(
      IBathToken(newBathToken).underlyingERC20() == overwriteERC20,
      "wrong bath token"
  );
  

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

2. Use strict equality in RubiconRouter

Payable functions in RubiconRouter use a loose check msg.value >= amount, which opens up the possibility to send more ETH than necessary, potentially causing users to lose fund when interacting with faulty front-end.

Recommended Mitigation

Consider using strict equality check:

require(msg.value == amount, "didnt send enough eth");

Affected Lines

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L336-L339
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L390-L393
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L462
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L504-L507

3. Unused param in BathToken.initialize

_feeTo parameter is not used in BathToken.initialize.

Recommended Mitigation

Consider removing the parameter if it's not meant to be used during initialisation. If used, update L217 to use the value.

4. Use call for ETH transfers

Using transfer() to send ETH is discouraged as it uses a hardcoded value of 2300 gas, which could cause problems when sending ETH to contracts that use more than 2300 gas in their fallback function. In addition, existing contracts that use less than 2300 gas now could potentially break if future hardforks modify the gas costs of their code.

A more detailed implications can be read here.

Affected Lines

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L356
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L374
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L434
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L451
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L491
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L548

Recommended Mitigation

Uses .call() instead when sending ETH.

5. rewardsVestingWallet cannot be initialised

Currently there is no way to initialise rewardsVestingWallet in BathToken.sol. benjamin has clarified that it will be added in the future.

6. Unbounded iteration over bonusTokens array

If the bonusTokens array grows big enough, the gas cost to execute distributeBonusTokenRewards could be higher than the block limit and the withdraw function is forever disabled.

Recommended Mitigation

Keep bonusTokens small by adding a hard limit or add a function to remove bonusTokens.

Non-critical Vulnerabilities

1. Use camel case for clarity and consistency

Struct naming typically use camel case, to avoid confusing from function and variable names. Change struct order to struct Order.
https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L61-L66

2. Remove unuseful comment

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