Gas Optimizations

[code423n4]

Table of Contents:

• Cheap Contract Deployment Through Clones
• Boolean comparisons
• Bytes constants are more efficient than string constants
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• Shift Right instead of Dividing by 2
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• Upgrade pragma to at least 0.8.4
• Reduce the size of error messages (Long revert Strings)
• Functions guaranteed to revert when called by normal users can be marked payable
• 10e18 is more gas efficient than 10**18

Cheap Contract Deployment Through Clones

See @audit tag:

File: BathHouse.sol
424:             TransparentUpgradeableProxy newBathToken
425:          = new TransparentUpgradeableProxy(
426:             newBathTokenImplementation,
427:             proxyManager,
428:             _initData
429:         );

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:

• Research clone factory to deploy erc20 bond contracts porter-finance/v1-core#15 (comment)
• Bond factory porter-finance/v1-core#34

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.
I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:

contracts/rubiconPools/BathHouse.sol:
  372:             approvedStrategists[wouldBeStrategist] == true ||

contracts/rubiconPools/BathToken.sol:
  228:             IBathHouse(bathHouse).isApprovedPair(msg.sender) == true,

Bytes constants are more efficient than string constants

From the Solidity doc:

If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.

Why do Solidity examples use bytes32 type instead of string?

bytes32 uses less gas because it fits in a single word of the EVM, and string is a dynamically sized-type which has current limitations in Solidity (such as can't be returned from a function to a contract).

If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity. Basically, any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.

Instances of string constant that can be replaced by bytes(1..32) constant :

contracts/peripheral_contracts/TokenWithFaucet.sol:
  16:     string public constant version = "1";

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

contracts/RubiconMarket.sol:
   400:         require(pay_amt > 0);
   402:         require(buy_amt > 0);
   985:         require(id > 0);
  1002:         require(id > 0);
  1175:         require(_span[pay_gem][buy_gem] > 0);

contracts/rubiconPools/BathHouse.sol:
  110          require(_reserveRatio <= 100);
  111:         require(_reserveRatio > 0);
  281:         require(rr > 0);

contracts/rubiconPools/BathPair.sol:
  332          require(
  333:             (askNumerator > 0 && askDenominator > 0) ||
  334:                 (bidNumerator > 0 && bidDenominator > 0),
  335              "one order must be non-zero"

Also, please enable the Optimizer.

Shift Right instead of Dividing by 2

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.

I suggest replacing / 2 with >> 1 here:

contracts/RubiconMarket.sol:
  77:         z = add(mul(x, y), WAD / 2) / WAD;
  81:         z = add(mul(x, y), RAY / 2) / RAY;
  85:         z = add(mul(x, WAD), y / 2) / y;
  89:         z = add(mul(x, RAY), y / 2) / y;

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration.
In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, I suggest storing the array's length in a variable before the for-loop, and use this new variable instead:

contracts/RubiconRouter.sol:
  169:         for (uint256 i = 0; i < route.length - 1; i++) {
  227:         for (uint256 i = 0; i < route.length - 1; i++) {

contracts/rubiconPools/BathPair.sol:
  311:         for (uint256 index = 0; index < array.length; index++) {
  582:         for (uint256 index = 0; index < ids.length; index++) {

contracts/rubiconPools/BathToken.sol:
  635:             for (uint256 index = 0; index < bonusTokens.length; index++) {

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

The same is also true for i--.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Affected code:

contracts/RubiconMarket.sol:
   436:         last_offer_id++;
  1165:         _span[address(pay_gem)][address(buy_gem)]++;
  1197:         _span[pay_gem][buy_gem]--;

contracts/RubiconRouter.sol:
   85:         for (uint256 index = 0; index < topNOrders; index++) {
  169:         for (uint256 i = 0; i < route.length - 1; i++) {
  227:         for (uint256 i = 0; i < route.length - 1; i++) {

contracts/rubiconPools/BathPair.sol:
  206:         last_stratTrade_id++;
  311:         for (uint256 index = 0; index < array.length; index++) {
  427:         for (uint256 index = 0; index < quantity; index++) {
  480:         for (uint256 index = 0; index < quantity; index++) {
  582:         for (uint256 index = 0; index < ids.length; index++) {

contracts/rubiconPools/BathToken.sol:
  635:             for (uint256 index = 0; index < bonusTokens.length; index++) {

Consider using ++i instead of i++ to increment the value of an uint variable. The same holds true with decrements (--i vs i--)

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
• Optimizer improvements in packed structs (>= 0.8.3)
• Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Consider upgrading pragma to at least 0.8.4

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

peripheral_contracts/BathBuddy.sol:45:            "VestingWallet: beneficiary is zero address"
peripheral_contracts/BathBuddy.sol:96:            "Caller is not the Bath Token beneficiary of these rewards"
peripheral_contracts/ERC20.sol:189:                "ERC20: transfer amount exceeds allowance"
peripheral_contracts/ERC20.sol:244:                "ERC20: decreased allowance below zero"
peripheral_contracts/ERC20.sol:269:        require(sender != address(0), "ERC20: transfer from the zero address");
peripheral_contracts/ERC20.sol:270:        require(recipient != address(0), "ERC20: transfer to the zero address");
peripheral_contracts/ERC20.sol:276:            "ERC20: transfer amount exceeds balance"
peripheral_contracts/ERC20.sol:313:        require(account != address(0), "ERC20: burn from the zero address");
peripheral_contracts/ERC20.sol:319:            "ERC20: burn amount exceeds balance"
peripheral_contracts/ERC20.sol:343:        require(owner != address(0), "ERC20: approve from the zero address");
peripheral_contracts/ERC20.sol:344:        require(spender != address(0), "ERC20: approve to the zero address");
peripheral_contracts/VestingWallet.sol:40:            "VestingWallet: beneficiary is zero address"
proxy/Address.sol:65:            "Address: unable to send value, recipient may have reverted"
proxy/Address.sol:129:                "Address: low-level call with value failed"
proxy/Address.sol:147:            "Address: insufficient balance for call"
proxy/Address.sol:173:                "Address: low-level static call failed"
proxy/Address.sol:188:        require(isContract(target), "Address: static call to non-contract");
proxy/Address.sol:209:                "Address: low-level delegate call failed"
proxy/Address.sol:224:        require(isContract(target), "Address: delegate call to non-contract");
proxy/TransparentUpgradeableProxy.sol:115:            "TransparentUpgradeableProxy: new admin is the zero address"
proxy/TransparentUpgradeableProxy.sol:176:            "TransparentUpgradeableProxy: admin cannot fallback to proxy target"
proxy/UpgradeableProxy.sol:81:            "UpgradeableProxy: new implementation is not a contract"
rubiconPools/BathHouse.sol:145:            "bathToken already exists for that ERC20"
rubiconPools/BathHouse.sol:149:            "bathToken does not exist for that desiredPairedAsset"
rubiconPools/BathHouse.sol:162:            "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
rubiconPools/BathHouse.sol:177:            "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
rubiconPools/BathHouse.sol:399:            "Cant create bathToken for zero address"
rubiconPools/BathHouse.sol:411:            "no implementation set for bathTokens"
rubiconPools/BathHouse.sol:417:            "initialize(address,address,address)",
rubiconPools/BathPair.sol:151:            "you are not an approved strategist - bathPair"
rubiconPools/BathPair.sol:180:            "Failed to meet asset pool reserve ratio"
rubiconPools/BathPair.sol:188:            "Failed to meet quote pool reserve ratio"
rubiconPools/BathPair.sol:318:        require(assigned, "Didnt Find that element in live list, cannot scrub");
rubiconPools/BathPair.sol:572:            "you are not the strategist that made this order"
rubiconPools/BathToken.sol:202:                    "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
rubiconPools/BathToken.sol:237:            "caller is not bathHouse - BathToken.sol"
rubiconPools/BathToken.sol:470:        require(_shares == shares, "did not mint expected share count");
rubiconPools/BathToken.sol:512:            "This implementation does not support non-sender owners from withdrawing user shares"
rubiconPools/BathToken.sol:518:            "You cannot withdraw the amount of assets you expected"
rubiconPools/BathToken.sol:549:            "This implementation does not support non-sender owners from withdrawing user shares"
RubiconMarket.sol:306:            "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee"
RubiconMarket.sol:310:            "_offer.pay_gem.transfer(msg.sender, quantity) failed"
RubiconMarket.sol:571:        require(isActive(id), "Offer was deleted or taken, or never existed.");
RubiconMarket.sol:574:            "Offer can not be cancelled because user is not owner, and market is open, and offer sells required amount of tokens."
RubiconRouter.sol:338:            "must send as much ETH as max_fill_withFee"
RubiconRouter.sol:392:            "didnt send enough native ETH for WETH offer"
RubiconRouter.sol:446:            "trying to cancel a non WETH order"
RubiconRouter.sol:506:            "must send enough native ETH to pay as weth and account for fee" 

I suggest shortening the revert strings to fit in 32 bytes.

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyAdmin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

10e18 is more gas efficient than 10**18

contracts/RubiconMarket.sol:
   73:     uint256 constant WAD = 10**18;
   74:     uint256 constant RAY = 10**27;
  857:                     pay_amt * 10**9,
  859:                 ) / 10**9;
  898:                         buy_amt * 10**9,
  900:                     ) / 10**9
  927:                 pay_amt * 10**9,
  929:             ) / 10**9
  951:                 buy_amt * 10**9,
  953:             ) / 10**9