Rubicon contest - 0x1f8b's results

Lines of code

https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L422-L428 https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L449

Vulnerability details

Impact

Anyone can cancel third party offers and take third party ether.

Proof of Concept

When offerForETH is called on RubiconRouter the RubiconMarket.offer method is called and an offer is created where the owner is the calling contract, RubiconMarket.

If you want to cancel it using cancelForETH the Market will check that the msg.sender is the owner in can_cancel modifier, always will be true, because the caller is RubiconRouter and the attacker will receive the original pay_amt.

Recommended Mitigation Steps

• RubiconMarket need to store the original owner.

Lines of code

https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L451

Vulnerability details

Impact

The user's ether could be locked and unrecoverable.

Proof of Concept

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, creates an offer, it will be impossible for this contract cancel the offer or receive funds back to that address.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected lines:

• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L548
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L491
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L451
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L434
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L374
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L356
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconMarket.sol#L361
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconMarket.sol#L309

Recommended Mitigation Steps

Use .call instead of .transfer

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L475

Vulnerability details

Impact

withdrawForETH method is vulnerable to token theft.

Description

The withdrawForETH method receives the targetPool argument from the user, the problem is that it completely trusts that said contract is trustworthy, however this contract can drain all the WETH9 that it has the contract prior to your call.

function withdrawForETH(uint256 shares, address targetPool) 
    external payable returns (uint256 withdrawnWETH)
{
    IERC20 target = IBathToken(targetPool).underlyingToken();
    require(target == ERC20(wethAddress), "target pool not weth pool");
    require(IBathToken(targetPool).balanceOf(msg.sender) >= shares,"don't own enough shares");
    IBathToken(targetPool).transferFrom(msg.sender, address(this), shares);
    withdrawnWETH = IBathToken(targetPool).withdraw(shares);
    WETH9(wethAddress).withdraw(withdrawnWETH);
    msg.sender.transfer(withdrawnWETH);
}

POC:

Here is an example of the exploit contract that would work as a targetPool. With this contract as targetPool, and 0 as shares, it's possible to drain all the reserves.

contract exploit
{
    address public WETH;
    uint256 public balance = 0;
    constructor(address w) { WETH=w; }

    function underlyingToken() public view returns (address) { return WETH; }
    function balanceOf(address a) public view returns (uint) { return 100000 ether; }
    function transferFrom(address a, address from, uint256 c) public returns (bool) 
    { 
        // calculate balance to drain
        balance = exploit(WETH).balanceOf(from);
        return true; 
    }
    function withdraw(uint256 a) public returns (uint) { return balance; }
}

Recommended Mitigation Steps

• Use targetPool whitelist

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

Lines of code

https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L157 https://etherscan.deth.net/address/0xdac17f958d2ee523a2206206994597c13d831ec7

Vulnerability details

Vulnerability details

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.

   function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

        // To change the approve amount you first have to reduce the addresses`
        //  allowance to zero by calling `approve(_spender, 0)` if it is not
        //  already 0 to mitigate the race condition described here:
        //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
        require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

        allowed[msg.sender][_spender] = _value;
        Approval(msg.sender, _spender, _value);
    }

Impact

The code as currently implemented does not handle these sorts of tokens properly, which would prevent USDT, the sixth largest pool, from being used by this project.

Affected source code:

• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L157
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/RubiconRouter.sol#L465
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathHouse.sol#L165
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathHouse.sol#L180
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathToken.sol#L214
• https://github.dev/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathToken.sol#L256

Recommended Mitigation Steps

• Call safeApprove with 0 amount first.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/peripheral_contracts/BathBuddy.sol#L69

Vulnerability details

Impact

Ether can get stuck in BathBuddy.

The BathBuddy contract is a copy of VestingWallet, however it does not allow ETH to be released (as VestingWallet did), but it allow to be received, so any ETH received will be burned and locked forever.

Recommended Mitigation Steps

• Remove receive() external payable {} from BathBuddy

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathToken.sol#L199-L210

Vulnerability details

Impact

The DOMAIN_SEPARATOR is wrong calculated.

Proof of Concept

In the initialize method of the BathToken contract, the name of the contract is used to calculate the DOMAIN_SEPARATOR, however said name is set later, so it will use an incorrect name, making it impossible to calculate the DOMAIN_SEPARATOR correctly.

DOMAIN_SEPARATOR = keccak256(
    abi.encode(
        keccak256(
            "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
        ),
        keccak256(bytes(name)),
        keccak256(bytes("1")),
        chainId,
        address(this)
    )
);
name = string(abi.encodePacked(_symbol, (" v1")));

Affected source code:

• BathToken.sol#L199-L210

Recommended Mitigation Steps

• Set the name before use it.

Lines of code

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

Vulnerability details

Impact

It's possible to steal tokens by the admin using a front-running attack in the BathToken contract.

Proof of Concept

The withdraw method and the redeem method do not establish the expected tokens to receive, and these depend on the fee established by the owner, this fee is not limited and allows passing the division factor (10_000), so a malicious admin could front-run a user, set a 100% fee (or higher and deny service) and receive more tokens than expected, stealing funds from the user.

Affected source code:

• BathToken.sol#L260

Recommended Mitigation Steps

• Limit the fee values
• Use TimeLock

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathToken.sol#L346-L369

Vulnerability details

Impact

Centralized risks allows rogue pool behavior.

Proof of Concept

The onlyPair modifier is as detailed below:

    modifier onlyPair() {
        require(
            IBathHouse(bathHouse).isApprovedPair(msg.sender) == true,
            "not an approved pair - bathToken"
        );
        _;
    }

And the bathHouse is the admin as you can see in the following comment in the initialize method

bathHouse = msg.sender; //NOTE: assumed admin is creator on BathHouse

So if the admin it's able to be the a valid pair (it could change the owner with setBathHouse), the owner it's able to call the method rebalance and steal any token.

Affected source code:

• BathToken.sol#L346-L369

Recommended Mitigation Steps

• Use timeLock, or avoid admin accounts.

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

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

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

Low

Inconsistent solidity pragmas

The source files have different solidity compiler ranges referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.

pragma solidity >= 0.5.0;
pragma solidity >= 0.7.6;
pragma solidity =  0.7.6;
pragma solidity >= 0.6.0 < 0.8.0;

Is not possible to deny strategies

The admin can approve strategies, but can never deny them, so it could affect the security of the site if a strategy is exploited and needs to be removed.

It is recommended to add a method to deny strategies.

Affected source code:

• BathHouse.sol#L264

Lack of input checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

• TokenWithFaucet.sol#L34
• BathToken.sol#L182-L184
• BathHouse.sol#L98-L102
• RubiconRouter.sol#L43-L44
• RubiconRouter.sol#L56-L58
• RubiconRouter.sol#L129
• Empty feeTo in RubiconMarket.sol#L556 and RubiconMarket.sol#L1256 could deny the service in buy method

It does not check that there are at least two routes and that they are different from each other.

Affected source code:

• RubiconRouter.sol#L164
• RubiconRouter.sol#L197

It is not verified that the fee is less than the factorial, so the service could be denied or the user's funds affected by the admin. You need to add the conditional fee <= 10_000.

Affected source code:

• RubiconMarket.sol#L1232

Unsafe ERC20 calls

The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Reference:

• eip-20

NOTES: The following specifications use syntax from Solidity 0.4.17 (or above) Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

approve:

• BathToken.sol#L214
• BathToken.sol#L256
• BathHouse.sol#L165
• BathHouse.sol#L180

transfer:

• BathToken.sol#L353-L357
• BathToken.sol#L605
• BathPair.sol#L601
• BathPair.sol#L615
• RubiconRouter.sol#L251
• RubiconRouter.sol#L303
• RubiconRouter.sol#L320
• RubiconRouter.sol#L348
• RubiconRouter.sol#L377
• RubiconRouter.sol#L406
• RubiconRouter.sol#L471

transferFrom:

• BathToken.sol#L565
• RubiconRouter.sol#L202
• RubiconRouter.sol#L274
• RubiconRouter.sol#L366
• RubiconRouter.sol#L419
• RubiconRouter.sol#L486

Reentrancy pattern

Although it has not been possible to exploit the reentrancy attack, the logic of the methods named below, make the changes of the validation flags after a method that allows reentrancy, it is convenient to modify the flags before the external calls to contracts.

Move the faucetCheck[msg.sender] changes before _mint in:

• TokenWithFaucet.sol#L45

Skipped argument can confuse the deployer

The _feeTo argument is ignored and address(this) is used regardless of the value set by the initialize method.

This can confuse the deployer and believe that it is using a value that it really is not, it is recommended to remove the argument or use the provided value.

Affected source code:

• BathToken.sol#L184

Integer overflow/underflow

In the handleStratOrderAtID method of the BathPair contract, an integer underflow occurs, because the pragma used allows it and safe math is not used, the following operation can produce an overflow.

uint256 askDelta = info.askPayAmt - offer1.pay_amt;
uint256 bidDelta = info.bidPayAmt - offer2.pay_amt;

It is believed that it cannot be exploited and take advantage of it, but this will require a deep study, So it's marked as low

Affected source code:

• BathPair.sol#L226-L227

Force to be different tokens

It is necessary to avoid two identical tokens in tokenPair. _underlyingAsset and _underlyingQuote must be different in:

• BathPair.sol#L339

In the following line if _underlyingAsset and _underlyingQuote are the same, they will use the same stratReward twice, affecting the logic of the contract in:

• BathPair.sol#L496-L497

Also asset and quote must be not equal in:

• RubiconRouter.sol#L56-L58
• RubiconRouter.sol#L129

In the method BathHouse.openBathTokenSpawnAndSignal, newBathTokenUnderlying and desiredPairedAsset could be the same which would emit an event that pairs two tokens that are not meant to be.

Affected source code:

• BathHouse.sol#L136

Front running createBathToken

You could fron-run createBathToken to create tokens with fee = address(0) if you call openBathTokenSpawnAndSignal prioritized.

This could affect the economics of the project because tokens would be created with fees not intended for what the owner initially wanted.

Affected source code:

• BathHouse.sol#L144-L148
• BathHouse.sol#L208

Emit breaking changes

In the BathHouse contract, it's possible to do an important hot changes and there is no emit detectable for dapps or users, so the owner could change important values and it could lead in rogue attack.

Affected source code:

• BathHouse.sol#L253-L337

Non-Critical

Imprecise comment

The comment mentions "quantity is not an int" but the reality is that it is, the precision of the integer should be specified, in this case int128.

Affected source code:

• RubiconMarket.sol#L282-L283

Avoid errors with transferFrom

The following methods take all the user's balance and send it through transferFrom, this call may fail, since transferFrom extracts the balance from the previously approved allowance, it is better to use the user's allowance in order to avoid the unnecessary failure when both amounts are not the same. It's better to use allowance instead of balanceOf.

Affected source code:

• RubiconRouter.sol#L273
• RubiconRouter.sol#L296
• RubiconRouter.sol#L313

Use npm packages instead of copy the dependencies

Using the packages from the original developer helps us stay up-to-date when new bugs appear.

IRubiconMarket.sol imports the openzeppelin libraries, as it should be, however these libraries are copied throughout the project.

Affected source code:

• IRubiconMarket.sol#L5

Here is not used open zeppelin imports:

• UpgradeableProxy.sol
• TransparentUpgradeableProxy.sol
• Address.sol

This cause a lot of duplicate names:

> Duplicate contract names found for IWETH.
> Duplicate contract names found for ERC20.
> Duplicate contract names found for IERC20.
> Duplicate contract names found for Address.
> Duplicate contract names found for TransparentUpgradeableProxy.
> Duplicate contract names found for UpgradeableProxy.

> This can cause errors and unknown behavior. Please rename one of your contracts.

Coding style - library not used

There are a SafeMath library but instead of importing, the methods are copied:

Affected source code:

• TokenWithFaucet.sol#L20-L26
• RubiconMarket.sol#L44-L90

Avoid use a contract for SafeMath

Reusing the code of the libraries can optimize the resulting script, since the compiler will only add the required code of the library once, adding the logic of these libraries in the contract, only makes the script more difficult to read and larger, with the consequent expense in gas.

Affected source code:

• RubiconMarket.sol#L44

Avoid unneeded modifiers

The can_offer modifier does not perform any action and can be ignored, this will reduce execution jumps.

Affected source code:

• RubiconMarket.sol#L221-L223

Use storage pointers or memory

Use storage keyword for save gas in order to cache a storage pointer.

The recurring use of the same register reading from storage is a task that can be easily optimized by using storage or memory, a clear example is access to offers[id] but it can be seen throughout all contracts.

Affected source code:

• RubiconMarket.sol#L260-L266

++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.

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 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• BathToken.sol#L635
• BathPair.sol#L311
• BathPair.sol#L427
• BathPair.sol#L480
• BathPair.sol#L582
• RubiconRouter.sol#L85
• RubiconRouter.sol#L169
• RubiconRouter.sol#L227

Change from public to external

To conserve gas and enhance code quality, the following routines could be made external.

The cost of an external call is less than that of a public function.

Affected source code:

• getExpectedSwapFill in RubiconRouter.sol#L166
• swap in RubiconRouter.sol#L199

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.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Affected source code:

• RubiconRouter.sol#L392
• ERC20.sol#L189
• ERC20.sol#L244
• ERC20.sol#L269-L270
• ERC20.sol#L276
• ERC20.sol#L313
• ERC20.sol#L319
• ERC20.sol#L343-L344
• VestingWallet.sol#L40
• BathBuddy.sol#L45
• BathBuddy.sol#L96
• BathToken.sol#L237
• BathToken.sol#L470
• BathToken.sol#L512
• BathToken.sol#L518
• BathToken.sol#L549
• BathPair.sol#L151
• BathPair.sol#L180
• BathPair.sol#L188
• BathPair.sol#L572
• BathHouse.sol#L145
• BathHouse.sol#L149
• BathHouse.sol#L162
• BathHouse.sol#L177
• BathHouse.sol#L399
• BathHouse.sol#L411
• RubiconRouter.sol#L338
• RubiconRouter.sol#L446
• RubiconMarket.sol#L306
• RubiconMarket.sol#L310
• RubiconMarket.sol#L571
• RubiconMarket.sol#L574

Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• RubiconMarket.sol#L375
• RubiconMarket.sol#L708
• RubiconMarket.sol#L722
• RubiconMarket.sol#L736
• RubiconMarket.sol#L750
• RubiconMarket.sol#L767
• RubiconMarket.sol#L1199
• RubiconMarket.sol#L1228
• RubiconMarket.sol#L1233
• RubiconMarket.sol#L1243
• RubiconMarket.sol#L1253
• RubiconMarket.sol#L1258

The isClosed method is immutable, consider removing the method or changing the logic in:

• RubiconMarket.sol#L471-L473

Dead code

The following contracts are never used. It's an excellent idea to take them out to save gas and improve the codding style.

Affected source code:

• SafeMathE.sol
• IERC20Minimal.sol
• VestingWallet.sol (Imported but never used)

The following variables are never used. Again, it's better to remove them in order to save gas and code readability.

Affected source code:

• wards in TokenWithFaucet.sol#L11
• EtherReleased event in BathBuddy.sol#L53
• _feeTo in BathToken.sol#L184
• stopped logic is never used in RubiconMarket.sol#L449

Reduce math operations

Use constants (e.g. type(uint256).max) rather than computation (e.g. 2**256-1).

Affected source code:

• BathToken.sol#L214
• BathToken.sol#L256
• BathToken.sol#L421
• BathToken.sol#L451
• RubiconRouter.sol#L157

Use scientific notation (e.g. 10E9) rather than exponentiation (e.g. 10**9).

Affected source code:

• RubiconMarket.sol#L857
• RubiconMarket.sol#L859
• RubiconMarket.sol#L898
• RubiconMarket.sol#L900
• RubiconMarket.sol#L927
• RubiconMarket.sol#L929
• RubiconMarket.sol#L951
• RubiconMarket.sol#L953

Reduce code complexity

Instead of using a contract, if a library is used, it is not necessary to increase the inheritance of contracts, improves readability, reduces auditing complications and makes the code less matriusca.

There are cases where a contract has been created just to store events, reducing inheritance helps make the code more readable and can save significant gas.

Affected source code:

• RubiconMarket.sol#L511-L525

Reduce boolean comparison

It's compared a boolean value using == true or == false, instead of using the boolean value. NOT opcode, it's cheaper to use EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Remove == true in:

• BathToken.sol#L228
• BathPair.sol#L149
• BathHouse.sol#L372

Change != true by ! in:

• BathHouse.sol#L242

Avoid continuous casting

Store bathHouse as IBathHouse to avoid continuous casting:

Affected source code:

• BathPair.sol#L168-L185

Logic optimization

Below are some logic changes that will improve the gas cost of the affected methods.

Remove uint256 expectedShares and use uint256 shares in withdraw.

Affected source code:

• BathToken.sol#L514

Change isApprovedPair from:

    function isApprovedPair(address pair) public view returns (bool outcome) {
        pair == approvedPairContract ? outcome = true : outcome = false;
    }

to

    function isApprovedPair(address pair) public view returns (bool outcome) {
        outcome = pair == approvedPairContract;
    }

in order to reduce some opcodes.

Affected source code:

• BathHouse.sol#L383

Remove assigned var in getIndexFromElement

    function getIndexFromElement(uint256 uid, uint256[] storage array)
        internal
        view
        returns (uint256)
    {
        uint l = array.length;
        for (uint256 index = 0; index < l; ++index) {
            if (uid == array[index]) return _index;
        }
        require(false, "Didnt Find that element in live list, cannot scrub");
    }

Affected source code:

• BathPair.sol#L305

Avoid useless variables

Change from:

uint256 _id = ids[index];
scrubStrategistTrade(_id);

to

scrubStrategistTrade(ids[index]);

Affected source code:

• BathPair.sol#L584

Use the returns variable newBathToken instead of create newOne

Affected source code:

• BathHouse.sol#L153

Use constant instead of storage

name = "Rubicon Bath House"; could be a constant because never will have a dynamic value, this will make it accessible even before it is initialized

Affected source code:

• BathHouse.sol#L105

Change bpsToStrategists to be constant with the value of 20:

• BathHouse.sol#L116