Rubicon contest - FSchmoede's results

QA report

[N-01] Initialize revert string

There is a inconsistent use of revert strings for the the different contracts’ initialize() methods. The contract RubiConMarket.sol is the only one having a revert string at L552:

require(!initialized, "contract is already initialized");

The same revert string should be used for the initialize() methods in the contracts:

• BathHouse.sol L104
• BathPair.sol L118
• BathToken.sol L186

[N-02] Remove out-commented code

In the contract RubiConMarket.sol at L93-L110 there is a whole contract commented out. If this is a left-over then delete it to remove any unused code.

[N-03] Inconsistent use of named return variables

There is a inconsistent use of named and unnamed return variables in the code base. A good example is two methods isApprovedStrategist() and isApprovedPair() in BathHouse.sol, which both do some checks and returns a bool. However, one of them uses a named return value, where the other does not. Since the methods are similar, there should be a consistent use.

The method isApprovedStrategist() found at L366 has the signature:

function isApprovedStrategist(address wouldBeStrategist) external view returns (bool)

And the method isApprovedPair() found at L382 has the signature:

function isApprovedPair(address pair) public view returns (bool outcome)

Either make both methods return (bool) or a named variable (bool outcome). This is applicable to many other occurrences in the code base.

Gas optimization report

[G-01] Optimize for loops

Several of the for-loops in the code can be optimized to cost less gas. One of the examples is the for-loop found in BathPair.sol at L311-L317:

for (uint256 index = 0; index < array.length; index++) {
    if (uid == array[index]) {
        _index = index;
        assigned = true;
        return _index;
    }
}

4 modifications can be done to this loop to save gas:

1. There is no need to initialize the counter i to 0, since this is automatically done when declaring a uint (the same argument as G-01).
2. Preincrement ++i is cheaper than postincrement i++ , since the latter will make use of a temporary variable to store the returned value in before incrementing it. This can be saved by doing a preincrement, which increments the value and then returns it.
3. The likelihood of a index inside a array is next to nothing, there is no need to use checked arithmetic, and this we can do unchecked arithmetic when incrementing the counter/array index i.
4. Cache the array length in a local variable outside of the loop, so it will only be evaluated once. Otherwise the length of the array has to be calculated for every iteration of the loop which is more expensive than just reading the local variable.

With the above suggestions the for-loop above can be rewritten into:

uint256 arrayLength = array.length;
for (uint256 index; index < arrayLength;) {
    if (uid == array[index]) {
        _index = index;
        assigned = true;
        return _index;
    }
		unchecked { ++index; }
}

[G-02] Revert strings below 32 bytes

If possible keep revert strings at a maximum of 32 bytes. Strings are stored in slots of 32 bytes, and hence the length of the revert string should be at max 32 bytes to fit inside 1 slot. If the string is just 33 bytes it will occupy 2 slots (64 bytes). Keeping the string size at 32 bytes or below will save gas on both deployment and when the revert condition is met.

Examples revert strings longer than 32 bytes:

RubiconMarket.sol L306: "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee"
RubiconMarket.sol L574: "Offer can not be cancelled because user is not owner, and market is open, and offer sells required amount of tokens."

RubiconRouter.sol L338: "must send as much ETH as max_fill_withFee"
RubiconRouter.sol L392: "didnt send enough native ETH for WETH offer"
RubiconRouter.sol L506: "must send enough native ETH to pay as weth and account for fee"

BathBuddy.sol L96: "Caller is not the Bath Token beneficiary of these rewards" 

The above is a non-exhaustive list and more occurrences can be found throughout the code.

[G-03] Don’t compare boolean to constant

It is more expensive to compare a boolean to a constant true/false than just evaluate the boolean it self, and both options will give the same result. Therefore, do not use == true or == false in requireor if statements.

An example can be found in BathToken.sol at L228:

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

This should instead be:

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

This should, with the optimizer set to 200 as in your code, save 6 gas. This is not a lot, but the case explained above was found quite a lot of places, and hence the total gas saving can be significant higher.

[G-04] Simplify isApprovedStrategist method

The method isApprovedStrategist() in the contract BathHouse.sol can be simplified, since it makes use of unneeded boolean constants.

The method can be found at L366-L379:

function isApprovedStrategist(address wouldBeStrategist)
    external
    view
    returns (bool)
{
    if (
        approvedStrategists[wouldBeStrategist] == true ||
        !permissionedStrategists
    ) {
        return true;
    } else {
        return false;
    }
}

Can be simplified into:

function isApprovedStrategist(address wouldBeStrategist)
    external
    view
    returns (bool)
{
		return (approvedStrategists[wouldBeStrategist] == true ||
        !permissionedStrategists)
}

This should, with the optimizer set to 200, save 9 gas per call to the method.

[G-05] Simplify isApprovedPair method

The method isApprovedPair() in the contract BathHouse.sol can be simplified, since it makes use of unneeded boolean constants.

The method can be found at L382-L384:

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

Can be simplified into:

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

This should, with the optimizer set to 200, save 24 gas per call to the method.

[G-06] Redundant modifier in scrubStrategistTrade method

The method scrubStrategistTrade() in the contract BathPair.sol has the modifier onlyApprovedStrategist(), however, this is redundant due to the remaining logic in the method.

The method can be found at L566-L575 and looks like this:

function scrubStrategistTrade(uint256 id)
    public
    onlyApprovedStrategist(msg.sender)
{
    require(
        msg.sender == strategistTrades[id].strategist,
        "you are not the strategist that made this order"
    );
    handleStratOrderAtID(id);
}

The require statement makes the modifier onlyApprovedStrategist() redundant, since the only way to get into the array strategistTrades[] is through the method placeMarketMakingTrades() , which also haves the onlyApprovedStrategist() modifier.

So there is no chance, that a user not being a approved strategist can get through the require statement above. Because of this the execution of the modifier can be left out to save some gas.

However, it is not possible to leave out the require statement instead of the modifier, since the user being a strategist of a trade implies the user is a approved strategist, but not vice versa.