Rubicon contest - joestakey's results

Lines of code

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

Vulnerability details

AVOID USING .TRANSFER TO TRANSFER ETH

In the RubiconRouter the .transfer() method is used at several times to transfer ETH.

If gas costs are subject to change, then smart contracts can’t depend on any particular gas costs.

Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300.

If an upgrade makes it so that a smart contract calling swapForETH's fallback function costs more than 2300 gas, the swaps will not work and a new Router will need to be deployed to avoid loss of funds.

Impact

Medium

Proof Of Concept

see this article

The .transfer method is called in these places:

msg.sender.transfer(delta)
msg.sender.transfer(buy_amt)
msg.sender.transfer(delta)
msg.sender.transfer(pay_amt)
msg.sender.transfer(withdrawnWETH)
msg.sender.transfer(fill)

Tools Used

Manual Analysis

Mitigation

use .call() to send ETH.

Lines of code

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

Vulnerability details

ERC20: TRANSFER() RETURN VALUE NOT CHECKED

the ERC20 .transfer() method is used in most functions of RubiconRouter.sol, but its return value is not checked. It is good to add a require() statement that checks the return value of token transfers, or to use something like OpenZeppelin’s safeTransfer. Failure to do so will cause silent failures of transfers, which can result in loss of funds.

Impact

Medium

Proof Of Concept

In maxBuyAllAmount, the function calls the RubiconMarket's buyAllAmount function before transferring the buy_gem token to the caller. If this ERC20 transfer silently fails, the function will not revert and the user will lose all their pay_gem tokens.

Tools Used

Manual Analysis

Mitigation

use safeTransfer or require() consistently.

Lines of code

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

Vulnerability details

NO TIMELOCK OR LIMIT ON SETFEEBPS()

In BathToken, there is no timelock or limit on setFeeBPS(). This is the fee that is applied in _withdraw(), and determines how much of his tokens the liquidity provider actually receives upon withdrawl. But:

• there is no maximum value limiting what fee can be set to (in setFeeBPS())
• there is no timelock, ie feeBPS is set to its new value when setFeeBPS() is created.

Users will be incited to deposit tokens in pools if the fee is low (feeRate is initially 0). A malicious BathHouse admin could effectively wait for liquidity providers to deposit tokens, then set a very high feeBPS and set feeToo to be his personal wallet address, which would result in any withdrawal sending all the liquidity provider's tokens to the malicious admin.

Impact

Medium

Proof Of Concept

• There is no limit on feeBPS in setFeeBPS
• A malicious BathHouse admin can set feeBPS so that feeTo receives all the tokens and the user does not receive any

Tools Used

Manual Analysis

Recommended Mitigation Steps

An appropriate upper limit should be put on setFeeBPS. The setFeeTo function could also perform more checks on the function argument being passed, as the protocol's tokenomics do depend on what address feeTo is.

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Commented code
• Event should be emitted in setters
• Events emitted early
• Event indexing
• Function missing comments
• Magic numbers
• Require statements should have descriptive strings
• Unchecked inputs
• Check zero denominator
• Initialize can be front run
• mint and burn missing zero address check

summary

The main concerns are with the front-run possibility in proxy contracts. In terms of non-critical issues, there is a large number of functions lacking comments.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BathPair.sol

function initialize, parameters: uint256 _maxOrderSizeBPS, int128 _shapeCoefNum function enforceReserveRatio, parameters: address underlyingAsset, address underlyingQuote function logFill, parameters: uint256 amt,address strategist, address asset function handleStratOrderAtID, parameter: uint256 id function getOfferInfo, parameter: uint256 id function getIndexFromElement, parameter: uint256 id, uint256[] storage array function requote, parameter: uint256 id function batchRequoteOffers, parameter: uint256[] memory ids function rebalancePair, parameter: address _underlyingAsset, address _underlyingQuote function tailOff, parameter: address targetPool, address tokenToHandle, address targetToken, uint24 _poolFee function scrubStrategistTrade, parameter: uint256 id function scrubStrategistTrades, parameter: uint256[] memory ids function scrubStrategistTrades, parameter: uint256[] memory ids function strategistBootyClaim, parameter: address asset, address quote function getOutstandingStrategistTrades, parameter: address asset, address quote, address strategist

BathHouse.sol

function initialize, parameters: address market, uint256 _reserveRatio, uint256 _timeDelay, address _newBathTokenImplementation, address _proxyAdmin function openBathTokenSpawnAndSignal, parameters: ERC20 newBathTokenUnderlying, uint256 initialLiquidityExistingBathToken function createBathToken, parameters: ERC20 underlyingERC20, address _feeAdmin function adminWriteBathToken, parameters: ERC20 overwriteERC20, address newBathToken function initBathPair, parameters: address _bathPairAddress, uint256 _maxOrderSizeBPS, int128 _shapeCoefNum function setBathHouseAdmin, parameters: address newAdmin function setNewBathTokenImplementation, parameters: address newImplementation function approveStrategist, parameters: address strategist function setPermissionedStrategists, parameters: bool _new function setCancelTimeDelay, parameters: uint256 value function setReserveRatio, parameters: uint256 rr function setBathTokenMarket, parameters: address bathToken, address newMarket function setBonusMarket, parameters: address bathToken, address newBonusToken function setBathTokenBathHouse, parameters: address bathToken, address newAdmin function setBathTokenFeeBPS, parameters: address bathToken, uint256 newBPS function bathTokenApproveSetMarket, parameters: address targetBathToken function setBathTokenFeeTo, parameters: address bathToken, address feeTo function setMarket, parameters: address newMarket function getBathTokenfromAsset, parameters: ERC20 asset) function isApprovedStrategist, parameters: address wouldBeStrategist function isApprovedPair, parameters: address pair

RubiconMarket.sol

function buy, parameters: uint256 id, uint256 quantity function cancel, parameters: uint256 id function offer, parameters: uint256 pay_amt, ERC20 pay_gem, uint256 buy_amt,ERC20 buy_gem function initialize, parameters: bool _live, address _feeTo function buy, parameters: uint256 id, uint256 amount function cancel, parameters: uint256 id function getBestOffer, parameters: ERC20 sell_gem, ERC20 buy_gem function getWorseOffer, parameters: uint256 id function getBetterOffer, parameters: uint256 id function getOfferCount, parameters: ERC20 sell_gem, ERC20 buy_gem function getNextUnsortedOffer, parameters: uin256 id

BathToken.sol

function initialize, parameters: ERC20 token, address market, address _feeTo function setMarket, parameters: address newRubiconMarket function setBathHouse, parameters: address newBathHouse function setFeeBPS, parameters: uint256 _feeBPS function setFeeTo, parameters: address _feeTo function setBonusToken, parameters: address newBonusERC20 function cancel, parameters: uint256 id, uint256 amt function removeFilledTradeAmount, parameters: uint256 amt function placeOffer, parameters: uint256 pay_amt, ERC20 pay_gem, uint256 buy_amt,ERC20 buy_gem function rebalance, parameters: address destination, uint256 stratProportion,uint256 rebalAmt function withdraw, parameters: uint256 _shares function withdraw, parameters: uint256 _shares, address receiver function distributeBonusTokenRewards, parameters: address receiver, uint256 sharesWithdrawn, uint256 initialTotalSupplyr

RubiconRouter.sol

function getBestOfferAndInfo, parameters: address asset, address quote function approveAssetOnMarket, parameters: address toApprove function getExpectedSwapFill, parameters: uint256 pay_amt, uint256 buy_amt_min function swap, parameters: uint256 pay_amt, uint256 buy_amt_min function swapEntireBalance, parameters: uint256 buy_amt_min, uint256 expectedMarketFeeBPS function maxBuyAllAmount, parameters: ERC20 buy_gem, ERC20 pay_gem, uint256 max_fill_amount function maxSellAllAmount, parameters: ERC20 pay_gem, ERC20 buy_gem, uint256 min_fill_amount function buyAllAmountWithETH, parameters: ERC20 buy_gem, uint256 buy_amt, uint256 max_fill_amount, uint256 expectedMarketFeeBPS function buyAllAmountForETH, parameters: uint256 buy_amt, ERC20 pay_gem, uint256 max_fill_amount function cancelForETH, parameters: uint256 id function depositWithETH, parameters: uint256 amount, address targetPool function withdrawWithETH, parameters: uint256 shares, address targetPool function swapWithETH, parameters: uint256 pay_amt, uint256 buy_amt_min, uint256 expectedMarketFeeBPS function swapForETH, parameters: uint256 pay_amt, uint256 buy_amt_min, uint256 expectedMarketFeeBPS

BathBuddy.sol

function released, parameters: address token function release, parameters: IERC20 token, address recipient, uint256 sharesWithdrawn, uint256 initialTotalSupply,uint256 poolFee function vestedAmount, parameters: address token, uint64 timestamp function _vestingSchedule, parameters: uint256 totalAllocation, uint64 timestamp

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Commented code

PROBLEM

There is commented code in the RubiconMarket.sol file

SEVERITY

Non-critical

PROOF OF CONCEPT

RubiconMarket.sol

TOOLS USED

Manual Analysis

MITIGATION

Delete commented code

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BathPair.sol

LogStrategistRewardClaim emitted before computations are done on storage variables, here and here

RubiconMarket.sol

LogSetOwner emitted before other storage variables are initialized

BathToken.sol

LogInit emitted before other storage variables are initialized

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BathPair.sol

event LogStrategistTrade event LogScrubbedStratTrade event LogStrategistRewardClaim

BathHouse.sol

event LogNewBathToken event LogOpenCreationSignal

RubiconMarket.sol

event LogItemUpdate(uint256 id) event LogInt(string lol, uint256 input) event OfferDeleted(uint256 id) event LogBuyEnabled(bool isEnabled) event LogMinSell(address pay_gem, uint256 min_amount) event LogMatchingEnabled(bool isEnabled) event LogUnsortedOffer(uint256 id) event LogSortedOffer(uint256 id) event LogInsert(address keeper, uint256 id) event LogDelete(address keeper, uint256 id) event LogMatch(uint256 id, uint256 amount)

BathToken.sol

event LogInit(uint256 timeOfInit) event LogDeposit event LogWithdraw event LogRebalance event LogPoolCancel event LogPoolOffer event LogRemoveFilledTradeAmount

BathBuddy.sol

event EtherReleased

TOOLS USED

Manual Analysis

MITIGATION

Add the indexed keyword to the variables emitted in the events mentioned above

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BathHouse.sol

setBathHouseAdmin setNewBathTokenImplementation setPermissionedStrategists setCancelTimeDelay setReserveRatio setMarket

RubiconMarket.sol

setFeeBPS setAqueductDistributionLive setAqueductAddress setFeeTo

BathToken.sol

setMarket setBathHouse setFeeBPS setFeeTo setBonusToken

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function comment is not correct

PROBLEM

There is an instance of a function where the comment does not correspond to what the function performs.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BathHouse.sol

setBathTokenMarket

/// @notice Admin-only function to set a Bath Token's timeDelay

TOOLS USED

Manual Analysis

MITIGATION

Replace the comment with an appropriate one.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

RubiconMarket.sol

function isActive function getOwner function getOffer function bump function kill function make function take function _next_id function isClosed function getTime function stop function make function take function kill function offer function isOfferSorted function setFeeBPS

RubiconRouter.sol

function startErUp, parameters: address _theTrap, address payable _weth

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Magic numbers

PROBLEM

Constants should be defined rather than using magic numbers

PROOF OF CONCEPT

RubiconMarket.sol

10**9 10**9 10**9 10**9 10**9 10**9 10**9 10**9

TOOL USED

Manual Analysis

MITIGATION

Use constants rather than magic numbers.

Require statements should have descriptive strings

PROBLEM

Some require statements are missing error strings, which makes it more difficult to debug when the function reverts.

PROOF OF CONCEPT

BathPair.sol

require(!initialized) require(msg.sender == bathHouse)

BathHouse.sol

require(msg.sender == admin) require(!initialized) require(_reserveRatio <= 100) require(_reserveRatio > 0) require(rr <= 100) require(rr > 0)

RubiconMarket.sol

require(isActive(id)) require(isActive(id)) require(getOwner(id) == msg.sender) require(!locked) require(_offer.pay_gem.transfer(_offer.owner, _offer.pay_amt)) require(cancel(uint256(id))) require(uint128(pay_amt) == pay_amt) require(uint128(buy_amt) == buy_amt) require(pay_amt > 0) require(pay_gem != ERC20(0x0)) require(buy_amt > 0) require(buy_gem != ERC20(0x0)) require(pay_gem != buy_gem) require(pay_gem.transferFrom(msg.sender, address(this), pay_amt)) require(buy(uint256(id), maxTakeAmount)) require(!isClosed()) require(isActive(id)) require(!isClosed()) require(isActive(id)) require((msg.sender == getOwner(id)) || isClosed()) require(buy(uint256(id), maxTakeAmount)) require(cancel(uint256(id))) require(_dust[address(pay_gem)] <= pay_amt) require(_unsort(id)) require(_hide(id)) require(!isOfferSorted(id)) require(isActive(id)) require(!isActive(id) && _rank[id].delb != 0 && _rank[id].delb < block.number - 10) require(offerId != 0) require(fill_amt >= min_fill_amount) require(offerId != 0) require(fill_amt <= max_fill_amount) require(offerId != 0) require(buyEnabled) require(super.buy(id, amount)) require(id > 0) require(id > 0) require(_dust[address(pay_gem)] <= pay_amt) require(isActive(id)) require(_span[pay_gem][buy_gem] > 0) require(_rank[id].delb == 0 &&isOfferSorted(id)) require(_rank[_rank[id].next].prev == id) require(_rank[_rank[id].prev].next == id) require(!isOfferSorted(id))

BathToken.sol

require(!initialized)

RubiconRouter.sol

require(!started)

TOOL USED

Manual Analysis

MITIGATION

Add error strings to all require statements.

Unchecked inputs

PROBLEM

There should be a non-zero (address or integer) check in all setters

SEVERITY

Non-Critical

PROOF OF CONCEPT

BathHouse.sol

setBathHouseAdmin is not checking if the address passed is not zero
setNewBathTokenImplementation is not checking if the address passed is not zero
setMarket is not checking if the address passed is not zero

RubiconMarket.sol

setOwner is not checking if the address passed is not zero
setAqueductAddress is not checking if the address passed is not zero
setFeeTo is not checking if the address passed is not zero

BathToken.sol

setMarket is not checking if the address passed is not zero
setBathHouse is not checking if the address passed is not zero
setFeeTo is not checking if the address passed is not zero

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks.

Check zero denominator

PROBLEM

When a division is computed, it must be ensured that the denominator is non-zero to prevent failure of the function call.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

RubiconMarket.sol

_offer.pay_amt

BathBuddy.sol

duration

TOOLS USED

Manual Analysis

MITIGATION

For the first one, add a non-zero check to _offer.pay_amt. For duration, as it is set during contract deployment, simply add a non-zero check in the constructor.

Initialize can be front run

PROBLEM

It is important that proxy contracts are deployed and initialized in the same transaction to avoid any malicious front-running.

However, the deployment does not follow this pattern when deploying the contracts. As a result, a malicious attacker could monitor the Ethereum blockchain for bytecode that matches the RubiconMarket or RubiconRouter contract and front-run the initialize() transaction.

Severity

Low

Tools Used

Manual Analysis

Recommended Mitigation Steps

For initializing proxy contracts, it is recommended to use a factory contract that deploys the proxy and calls initialize in the same transaction.

mint and burn missing zero address check

IMPACT

mint() and burn() should check that the to and from are not the zero address.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

BathToken.sol

_mint() _burn()

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check in both functions

Gas Report

Table of Contents

• Boolean comparison
• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparisons with zero for unsigned integers
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Event emitting of local variable
• Group logic in modifier
• Immutable variables can save gas
• Magic Numbers
• Prefix increments
• Redundant getters
• Redundant overflow and underflow checks
• Require instead of AND
• Shifting cheaper than division
• Tight variable packing
• unchecked arithmetic
• unnecessary computation

Boolean comparison

IMPACT

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value

PROOF-OF-CONCEPT

Instances include:

BathHouse.sol

if (approvedStrategists[wouldBeStrategist] == true ||!permissionedStrategists)

BathToken.sol

require(IBathHouse(bathHouse).isApprovedPair(msg.sender) == true,"not an approved pair - bathToken")

TOOLS USED

Manual analysis

MITIGATION

Check the return boolean value directly. Replace (bool == true) with (bool)

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances include:

BathHouse.sol

scope: _createBathToken()

• newBathTokenImplementation is read twice:

BathHouse.sol:410 BathHouse.sol:426

BathPair.sol

scope: enforceReserveRatio()

• bathHouse is read four times:

BathPair.sol:168 BathPair.sol:171 BathPair.sol:177 BathPair.sol:185

scope: handleStratOrderAtID()

• bathHouse is read four times:

BathPair.sol:218 BathPair.sol:221 BathPair.sol:177 BathPair.sol:185

scope: getIndexFromElement()

• array.length is read (array.length) times: number of reads depending on array.length as it is in a for loop

BathPair.sol:311

RubiconMarket.sol

scope: bump()

• offers[id].pay_gem is read twice:

RubiconMarket.sol:260 RubiconMarket.sol:262

• offers[id].buy_gem is read twice:

RubiconMarket.sol:260 RubiconMarket.sol:263

scope: _hide()

• _head is read twice:

RubiconMarket.sol:1206 RubiconMarket.sol:1211

BathToken.sol

scope: convertToShares()

• totalSupply is read twice:

BathToken.sol:399 BathToken.sol:400

scope: previewMint()

• totalSupply is read twice:

BathToken.sol:457 BathToken.sol:459

scope: _deposit()

• underlyingToken is read four times:

BathToken.sol:562 BathToken.sol:565 BathToken.sol:566 BathToken.sol:577

• totalSupply is read three times:

BathToken.sol:569 BathToken.sol:570 BathToken.sol:582

scope: _withdraw()

• feeTo is read twice:

BathToken.sol:601 BathToken.sol:602

• underlyingToken is read three times:

BathToken.sol:602 BathToken.sol:605 BathToken.sol:609

scope: distributeBonusTokenRewards()

• bonusTokens.length is read 1 + (bonusTokens.length) times: number of reads depending on bonusTokens.length as it is in a for loop

BathToken.sol:634 BathToken.sol:635

BathBuddy.sol

scope: _vestingSchedule()

• start is read three times:

BathBuddy.sol:154 BathBuddy.sol:156 BathBuddy.sol:159

• duration is read twice:

BathBuddy.sol:156 BathBuddy.sol:159

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

BathPair.sol

address[2] memory tokenPair address[2] memory tokenPair uint256[] memory askNumerators uint256[] memory askDenominators uint256[] memory bidNumerators uint256[] memory bidDenominators uint256[] memory ids

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparisons with zero for unsigned integers

IMPACT

>0 is less gas efficient than != 0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes here

PROOF OF CONCEPT

Instances include:

BathHouse.sol

require(_reserveRatio > 0); require(rr > 0);

BathPair.sol

require((askNumerator > 0 && askDenominator > 0) ||(bidNumerator > 0 && bidDenominator > 0),"one order must be non-zero")

RubiconMarket.sol

require(pay_amt > 0) require(buy_amt > 0) require(id > 0) require(id > 0) require(_span[pay_gem][buy_gem] > 0)

TOOLS USED

Manual Analysis

MITIGATION

Replace > 0 with != 0

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

BathHouse.sol

require(_reserveRatio <= 100) require(rr <= 100)

BathPair.sol

.div(100) <= IERC20(underlyingAsset).balanceOf(bathAssetAddress) .div(100) <= IERC20(underlyingAsset).balanceOf(bathAssetAddress)

RubiconMarket.sol

_dust[address(pay_gem)] <= pay_amt pay_amt >= offers[offerId].buy_amt fill_amt >= min_fill_amount buy_amt >= offers[offerId].pay_amt fill_amt <= max_fill_amount t_pay_amt >= _dust[address(t_pay_gem)] _dust[address(pay_gem)] <= pay_amt

BathToken.sol

assetsReceived >= assets deadline >= block.timestamp

RubiconRouter.sol

currentAmount >= buy_amt_min currentAmount >= buy_amt_min msg.value >= max_fill_withFee msg.value >= pay_amt msg.value >= amount IBathToken(targetPool).balanceOf(msg.sender) >= shares msg.value >= amtWithFee

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-require(rr <= 100);
+require(rr < 101);

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

BathBuddy.sol

BathBuddy.sol

TOOLS USED

Manual Analysis

MITIGATION

Hardcode state variables with their initial value instead of writing them during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

BathHouse.sol

require(!initialized)
require(_reserveRatio <= 100)
require(_reserveRatio > 0)
require(getBathTokenfromAsset(newBathTokenUnderlying) == address(0), "bathToken already exists for that ERC20")
require(getBathTokenfromAsset(desiredPairedAsset) != address(0),"bathToken does not exist for that desiredPairedAsset")
require(newBathTokenUnderlying.transferFrom( msg.sender, address(this),initialLiquidityNew),"Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol")
require(desiredPairedAsset.transferFrom( msg.sender,address(this),initialLiquidityExistingBathToken),"Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol")
require(approvedPairContract == address(0),"BathPair already approved")
require(IBathPair(_bathPairAddress).initialized() != true,"BathPair already initialized")
require(rr <= 100)
require(rr > 0)
require(initialized, "BathHouse not initialized")
require( _underlyingERC20 != address(0),"Cant create bathToken for zero address")
require(tokenToBathToken[_underlyingERC20] == address(0),"bathToken already exists")
require(newBathTokenImplementation != address(0),"no implementation set for bathTokens")

BathPair.sol

require(!initialized)
require(IBathHouse(_bathHouse).getMarket() !=address(0x0000000000000000000000000000000000000000) && IBathHouse(_bathHouse).initialized(),"BathHouse not initialized")
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")
require(assigned, "Didnt Find that element in live list, cannot scrub")
require((askNumerator > 0 && askDenominator > 0) ||(bidNumerator > 0 && bidDenominator > 0),"one order must be non-zero")
require(bathAssetAddress != address(0) && bathQuoteAddress != address(0), "tokenToBathToken error")
require(askNumerators.length == askDenominators.length &&askDenominators.length == bidNumerators.length &&bidNumerators.length == bidDenominators.length,"not all order lengths match")
require(askNumerators.length == askDenominators.length &&askDenominators.length == bidNumerators.length &&bidNumerators.length == bidDenominators.length&& ids.length == askNumerators.length,"not all input lengths match")
require(_bathAssetAddress != address(0) && _bathQuoteAddress != address(0), "tokenToBathToken error") require(msg.sender == strategistTrades[id].strategist,"you are not the strategist that made this order")\

RubiconMarket.sol

require(uint128(spend) == spend, "spend is not an int")
require(uint128(quantity) == quantity, "quantity is not an int")
require( _offer.buy_gem.transferFrom(msg.sender, feeTo, fee),"Insufficient funds to cover fee")
require(_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend), "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee")
require(_offer.pay_gem.transfer(msg.sender, quantity),"_offer.pay_gem.transfer(msg.sender, quantity) failed")
require(_offer.pay_gem.transfer(_offer.owner, _offer.pay_amt))
require(cancel(uint256(id)))
require(uint128(pay_amt) == pay_amt)
require(uint128(buy_amt) == buy_amt)
require(pay_amt > 0)
require(pay_gem != ERC20(0x0))
require(buy_amt > 0)
require(buy_gem != ERC20(0x0))
require(pay_gem != buy_gem)
require(pay_gem.transferFrom(msg.sender, address(this), pay_amt))
require(buy(uint256(id), maxTakeAmount))
require(!initialized, "contract is already initialized")
require(buy(uint256(id), maxTakeAmount))
require(cancel(uint256(id)))
require(!locked, "Reentrancy attempt")
require(!locked, "Reentrancy attempt")
require(_dust[address(pay_gem)] <= pay_amt)
require(!locked, "Reentrancy attempt")
require(!locked, "Reentrancy attempt")
require(_unsort(id))
require(_hide(id))
require(!locked, "Reentrancy attempt")
require(!isOfferSorted(id))
require(isActive(id))
require(!locked, "Reentrancy attempt")
require(!isActive(id) && _rank[id].delb != 0 && _rank[id].delb < block.number - 10)
require(!locked, "Reentrancy attempt") require(offerId != 0)
require(fill_amt >= min_fill_amount)
require(!locked, "Reentrancy attempt")
require(offerId != 0)
require(fill_amt >= min_fill_amount)
require(offerId != 0)
require(offerId != 0)
require(buyEnabled)
require(super.buy(id, amount))
require(id > 0)
require(id > 0)
require(_dust[address(pay_gem)] <= pay_amt)
require(isActive(id))
require(_span[pay_gem][buy_gem] > 0) require( _rank[id].delb == 0 && isOfferSorted(id)) require(_rank[_rank[id].next].prev == id)
require(_rank[_rank[id].prev].next == id)
require(!isOfferSorted(id))

BathToken.sol

require(!initialized)
require(_shares == shares, "did not mint expected share count")
require(owner == msg.sender,"This implementation does not support non-sender owners from withdrawing user shares")
require(assetsReceived >= assets,"You cannot withdraw the amount of assets you expected")
require(owner == msg.sender,"This implementation does not support non-sender owners from withdrawing user shares")
require(deadline >= block.timestamp, "bathToken: EXPIRED")
require(recoveredAddress != address(0) && recoveredAddress == owner,"bathToken: INVALID_SIGNATURE")

RubiconRouter.sol

require(!started)
require(currentAmount >= buy_amt_min, "didnt clear buy_amt_min")
require(currentAmount >= buy_amt_min, "didnt clear buy_amt_min")
require(msg.value >= max_fill_withFee,"must send as much ETH as max_fill_withFee")
require(msg.value >= pay_amt, "didnt send enough native ETH for WETH offer")
require(address(pay_gem) == wethAddress,"trying to cancel a non WETH order")
require(target == ERC20(wethAddress), "target pool not weth pool")
require(msg.value >= amount, "didnt send enough eth")
require(target == ERC20(wethAddress), "target pool not weth pool")
require(IBathToken(targetPool).balanceOf(msg.sender) >= shares, "don't own enough shares")
require(route[0] == wethAddress, "Initial value in path not WETH")
require(msg.value >= amtWithFee,"must send enough native ETH to pay as weth and account for fee")
require(route[route.length - 1] == wethAddress,"target of swap is not WETH")
require(ERC20(route[0]).transferFrom(msg.sender,address(this),pay_amt.add(pay_amt.mul(expectedMarketFeeBPS).div(10000))),"initial ERC20 transfer failed")

BathBuddy.sol

require(beneficiaryAddress != address(0),"VestingWallet: beneficiary is zero address")
require(msg.sender == beneficiary,"Caller is not the Bath Token beneficiary of these rewards")

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in BathHouse.sol:

Replace

require(rr <= 100);

with

if (rr > 100) {
	revert ReserveRatioTooHigh(rr);
}

and define the custom error in the contract

error ReserveRatioTooHigh(uint256 rr);

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

BathPair.sol

bool assigned = false
uint256 index = 0
uint256 index = 0
uint256 index = 0
uint256 index = 0

RubiconMarket.sol

uint256 old_top = 0

BathToken.sol

uint256 index = 0

RubiconRouter.sol

uint256 lastBid = 0
uint256 lastAsk = 0
uint256 index = 0
uint256 currentAmount = 0
uint256 i = 0
uint256 currentAmount = 0
uint256 i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

RubiconMarket.sol

feeToo
buyEnabled
matchingEnabled

BathToken.sol

underlyingToken, totalSupply
underlyingToken, feeTo, totalSupply

TOOLS USED

Manual Analysis

MITIGATION

Emit the function argument instead of the storage variable to save gas. Or when appropriate, save the storage variable in a local variable, and emit the local variable instead (see this paragraph)

Group logic in modifier

PROBLEM

You can group repetitive require statements into a modifier to save on deployment costs. There is however a tradeoff to doing this: declaring the logic inline is cheaper than having it in a modifier when you call the function that uses that modifier.

PROOF OF CONCEPT

Instances include:

RubiconMarket.sol

require(!locked, "Reentrancy attempt") require(!locked, "Reentrancy attempt") require(!locked, "Reentrancy attempt") require(!locked, "Reentrancy attempt") require(!locked, "Reentrancy attempt") require(!locked, "Reentrancy attempt") require(!locked, "Reentrancy attempt") require(!locked, "Reentrancy attempt")

TOOLS USED

Manual Analysis

MITIGATION

You can create a modifier onlyUnlocked and apply it to the functions where the require statement mentioned above is called. Again, this one is more of a suggestion because there is a trade-off.

Immutable variables can save gas

PROBLEM

If a storage variable is never modified, marking it as immutable can save gas, as it is saved as a constant instead of a storage variable, saving a SLOAD operation (100 gas) every time the variable is read.

PROOF OF CONCEPT

Instances include:

RubiconRouter.sol

wethAddress is initialized once and can not be modified afterwards. It is read 22 times in total in the contract.

TOOLS USED

Manual Analysis

MITIGATION

add the immutable keyword to address payable public wethAddress

Magic Numbers

PROBLEM

To compute the maximum uint256, the use of computations on magic numbers is not as gas efficient as using the notation type(uint256).max. 25 gas can be saved every time approveAssetOnMarket() is called

PROOF OF CONCEPT

Instances include:

RubiconRouter.sol

ERC20(toApprove).approve(RubiconMarketAddress, 2**256 - 1

BathToken.sol

IERC20(address(token)).approve(RubiconMarketAddress, 2**256 - 1)
underlyingToken.approve(RubiconMarketAddress, 2**256 - 1)
maxAssets = 2**256 - 1
maxShares = 2**256 - 1

TOOLS USED

Manual Analysis

MITIGATION

Replace

2**256 - 1

with

type(uint256).max

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

BathPair.sol

last_stratTrade_id++
index++
index++
index++
index++

RubiconMarket.sol

last_offer_id++
_span[address(pay_gem)][address(buy_gem)]++

BathToken.sol

index++
nonces[owner]++

RubiconRouter.sol

index++
i++
i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Redundant getters

IMPACT

Every public state variable has a getter function created for it by the compiler. Not only does creating an additional getter function for these variables is useless and wastes gas upon contract deployment (with the optimizer enabled and set to 10000, it costs approximately 4000 extra gas per getter), but calling these getters also cost more gas than the built-in getter (~3500 gas more)

PROOF OF CONCEPT

Instances include

BathHouse.sol

getMarket()
getReserveRatio()
getCancelTimeDelay()
getBPSToStrats()

RubiconMarket.sol

getFirstUnsortedOffer()

TOOLS USED

Manual Analysis, Remix

MITIGATION

Remove the getters mentioned above.

Redundant overflow and underflow checks

IMPACT

As of Solidity 0.8.0, underflow and overflow checks are performed by default on additions and subtractions. Using an external library for these computations should be avoided as it will waste gas.

PROOF OF CONCEPT

Several contracts in scope use the SafeMath library of OpenZeppelin and its functions sub and add

TOOLS USED

Manual Analysis

MITIGATION

Upgrade the Solidity versions of the contracts to at least 0.8.0 and use the native additions and subtractions.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

BathPair.sol

require(IBathHouse(_bathHouse).getMarket() !=address(0x0000000000000000000000000000000000000000) &&IBathHouse(_bathHouse).initialized(),"BathHouse not initialized")
require((askNumerator > 0 && askDenominator > 0) ||(bidNumerator > 0 && bidDenominator > 0),"one order must be non-zero")
require(bathAssetAddress != address(0) && bathQuoteAddress != address(0),"tokenToBathToken error")
require(askNumerators.length == askDenominators.length &&askDenominators.length == bidNumerators.length &&bidNumerators.length == bidDenominators.length,"not all order lengths match")
require(askNumerators.length == askDenominators.length &&askDenominators.length == bidNumerators.length &&bidNumerators.length == bidDenominators.length && ids.length == askNumerators.length,"not all input lengths match")
require(_bathAssetAddress != address(0) && _bathQuoteAddress != address(0),"tokenToBathToken error")

RubiconMarket.sol

require(!isActive(id) &&_rank[id].delb != 0 &&_rank[id].delb < block.number - 10)

BathToken.sol

require(recoveredAddress != address(0) && recoveredAddress == owner,"bathToken: INVALID_SIGNATURE")

TOOLS USED

Manual Analysis

MITIGATION

Split the require statement in several statements. For instance:

-require(
            _bathAssetAddress != address(0) && _bathQuoteAddress != address(0),
            "tokenToBathToken error"
        )
+require(_bathAssetAddress != address(0))
+require(_bathQuoteAddress != address(0))

To improve the savings, you can also use custom errors instead of require statements

Shifting cheaper than division

IMPACT

A division by 2 can be calculated by shifting one to the right. While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

PROOF OF CONCEPT

Instances include:

RubiconMarket.sol

z = add(mul(x, y), WAD / 2
z = add(mul(x, y), RAY / 2)
z = add(mul(x, WAD), y / 2)
z = add(mul(x, RAY), y / 2)

TOOLS USED

Manual Analysis

MITIGATION

-z = add(mul(x, y), WAD / 2)
+z = add(mul(x, y), WAD >> 1)

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As bool type variables are of size 1 byte, there's a slot here that can get saved by moving them closer to an address

PROOF OF CONCEPT

Instances include:

BathHouse.sol

bool public initialized, bool public permissionedStrategists

RubiconMarket.sol

bool locked

bool public AqueductDistributionLive

BathToken.sol

bool public initialized

TOOLS USED

Manual Analysis

MITIGATION

Place booleans after an address to save on storage slot. For instance, replace:

address public proxyManager;

    /// @notice The core Rubicon Market of the Pools system
    address public RubiconMarketAddress;

    /// @notice A mapping of approved strategists to access Pools liquidity
    mapping(address => bool) public approvedStrategists;

    /// @notice The initialization status of BathHouse
    bool public initialized;

    /// @notice If true, strategists are permissioned and must be approved by admin
    bool public permissionedStrategists;

with

address public proxyManager;


    /// @notice The initialization status of BathHouse
    bool public initialized;

    /// @notice The core Rubicon Market of the Pools system
    address public RubiconMarketAddress;


    /// @notice If true, strategists are permissioned and must be approved by admin
    bool public permissionedStrategists;


    /// @notice A mapping of approved strategists to access Pools liquidity
    mapping(address => bool) public approvedStrategists;

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

RubiconRouter.sol

ERC20(buy_gem).transfer(msg.sender, _after - _before), _after > _before because of the check line 404, underflow check is unnecessary.
uint256 delta = _after - _before;, _after > _before because of the check line 404, underflow check is unnecessary.

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

In BathPair.sol and RubiconMarket.sol, the return statements and the increment can be combined in one statement, saving over 100 gas.

PROOF OF CONCEPT

Instances include:

BathPair.sol

scope: _next_id()

BathPair.sol:206-207

RubiconMarket.sol

scope: _next_id()

RubiconMarket.sol:436-437

TOOLS USED

Manual Analysis

MITIGATION

Replace

last_stratTrade_id++;
return last_stratTrade_id;

with

return(last_stratTrade_id++);

Note: the statement can be even more optimized by changing the location of the increment, see this paragraph