Rubicon contest - catchup's results

Lines of code

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

Vulnerability details

Impact

Admin is allowed to make critical updates such as the Bath token fee, reserve ratio, time delay, etc. Such changes may effect user decisions, hence they should have some safeguards such as timelocks and/or limits. Even if there is no bad intention, this kind of excessive power given to the admin can potentially be used to damage the reputation of the 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

Tools Used

Manual review

Recommended Mitigation Steps

• A timelock can be added for such critical changes which may effect user decisions.
• Min/Max limits can be defined for fees, reserve ratio, etc, which updates are checked against and reverted if not within limits.

It would be better to make admin change as a two-step process

setBathHouseAdmin() is called by the current admin to change the admin address. It can be a better approach to follow a 2-step process when updating such priviliged addresses First transaction proposes the pending admin address, second transaction which can only be called by the proposed address accepts the role.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L253-L255

Missing non-zero address check

It is a good practice to include non-zero address check especially when updating important addresses. I suggest to include non-zero address checks when setting addresses such as admin, strategist, fee recepient, etc.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L253 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L264 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L327

Important changes should emit events

Functions which are executed only by privileged users and change important parameters should emit events.

Lines of code

All the onlyAdmin functions in BathHouse.sol

Old solidity compiler version

Most of the contracts are using 0.7.6 compilers which are a bit outdated. It would be better to use a version >= 0.8.4 to benefit from new features such as builtin safemath operations and bugfixes.

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L310 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L311 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L427 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L480 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L582 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L635 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L82-L83 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L85 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L168 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L169 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L226 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L227 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L990

Prefix increment/decrements are cheaper than postfix increment/decrements

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L206 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L311 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L427 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L480 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L582 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L635 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L436 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L1165 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L85 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L169 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L227

Using != 0 is cheaper than > 0 when used on a uint in a require() statement

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L111 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L281 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L400 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L402 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L985 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L1002 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L1175

Error string longer than 32 characters

Error strings take space in the deployed bytecode. Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.

Lines of code

There are many instances, some of them are below: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/proxy/Address.sol#L65 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/proxy/Address.sol#L147 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/proxy/Address.sol#L188 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/proxy/Address.sol#L224 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L45 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L96 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L145 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L149 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L162 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L177

Cache the storage variables if they are going to be accessed multiple times

If a state variable is going to be accessed multiple times in a function, it is better to cache it and use it from the stack. SLOAD costs ~100 gas, so 100 gas would be saved for each extra read. This is especially more important if the state variable is used within a loop, since there can be many extra reads there.

Lines of code

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

memory arguments can be changed to calldata

It is cheaper to use calldata for the read-only arguments of external functions because then intermediate memory operations are avoided.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L578 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L413-L417 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L464-L469