Rubicon contest - c3phas's results

Different pragmas

contracts under peripheral (bathbuddy)use pragma solidity >=0.6.0 <0.8.0; while in the core contracts we have pragma solidity =0.7.6;

Natspec incomplete

To make the contract clearer to users and avoid some misunderstandings, it is recommended to use natspec for any public/external functions/variables in the code and tools can use that to display it to end users

File: Bathhouse.sol line 388,

    /// @dev Low-level functionality to spawn a Bath Token using the OZ Transparent Upgradeable Proxy standard
    /// @param underlyingERC20 The underlying ERC-20 asset that underlies the newBathTokenAddress
    /// @param _feeAdmin Recipient of pool withdrawal fees, typically the pool itself
    function _createBathToken(ERC20 underlyingERC20, address _feeAdmin)
        internal
        returns (address newBathTokenAddress)
    {

Missing @return

File: Bathtoken.sol line 754

    /// @notice The best-guess total claim on assets the Bath Token has
    /// @dev returns the amount of underlying ERC20 tokens in this pool in addition to any tokens that are outstanding in the Rubicon order book seeking market-making yield (outstandingAmount)
    function underlyingBalance() public view returns (uint256) {
        uint256 _pool = IERC20(underlyingToken).balanceOf(address(this));
        return _pool.add(outstandingAmount);

missing @return

File: Bathpair.sol line 158

    /// @notice This function enforces that the Bath House reserveRatio (a % of underlying pool liquidity) is enforced across all pools
    /// @dev This function should ensure that reserveRatio % of the underlying liquidity always remains on the Bath Token. Utilization should be 1 - reserveRatio in practice assuming strategists use all available liquidity.
    function enforceReserveRatio(
        address underlyingAsset,
        address underlyingQuote
    )
        internal
        view
        returns (address bathAssetAddress, address bathQuoteAddress)

Missing @param and @return

File: BathPair.sol line 303

    /// @notice A function that returns the index of uid from array
    /// @dev uid must be in array for the purposes of this contract to enforce outstanding trades per strategist are tracked correctly
    function getIndexFromElement(uint256 uid, uint256[] storage array)
        internal
        view
        returns (uint256 _index)

Missing @param and @return

File: BathPair.sol line 629

    /// @notice The goal of this function is to enable a means to retrieve all outstanding orders a strategist has live in the books
    /// @dev This is helpful to manage orders as well as track all strategist orders (like their RAM of StratTrade IDs) and place any would-be constraints on strategists
    function getOutstandingStrategistTrades(
        address asset,
        address quote,
        address strategist
    ) public view returns (uint256[] memory) {
        return outOffersByStrategist[asset][quote][strategist];
    }
}

Missing @param and @return

Inconsistency with Natspec

In bathtoken.sol and most of the other contracts, natspec is used irregulary, some function have it some don't.

FINDINGS

Use Short revert strings

We always need to use strings along with our require statements to explain why an error happened. These strings, however, 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.

Revert strings that are longer than 32 bytes require at least one additional mstore along with an additional overhead for computing memory offset. Having a short one will decrease deployment gas and decrease the runtime gas when the revert condition is met

File: RubiconMarket.sol: line 571

File: RubiconMarket.sol: line 572

// After close, anyone can cancel an offer
    modifier can_cancel(uint256 id) override {
        require(isActive(id), "Offer was deleted or taken, or never existed.");
        require(
            isClosed() || msg.sender == getOwner(id) || id == dustId,
            "Offer can not be cancelled because user is not owner, and market is open, and offer sells required amount of tokens."
        );
        _;
    }

File: RubiconMarket.sol: line 304,

File: RubiconMarket.sol: line 308

File: RubiconRouter.sol: line 336

File: RubiconRouter.sol: line 390

File: RubiconRouter.sol: line 444

File: RubiconRouter.sol: line 504

File: BathHouse.sol: line 143

File: BathHouse.sol: line 147

File: BathHouse.sol: line 156

File: BathHouse.sol: line 171

File: BathHouse.sol: line 397

File: BathPair.sol: line 148

File: BathPair.sol: line 174

File: BathPair.sol: line 182

File: BathPair.sol: line 318

File: BathPair.sol: line 570

File: BathToken.sol line 227

File: BathToken.sol line 470

File: BathToken.sol line 510

File: BathToken.sol line 516

File: BathToken.sol line 547

BathBuddy.sol line 43

BathBuddy.sol line 94

Split require statement using && which saves us around 8 gas

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 3 GAS per &&:

File: RubiconMarket.sol: line 715

File: RubiconMarket.sol: line 1177

File: BathPair.sol line 120

File: BathPair.sol line 332

File: BathPair.sol line 346

File: BathPair.sol line 419

File: BathPair.sol line 471

File: BathPair.sol line 506

File: BathToken.sol line 740

Use != 0 instead of > 0 for unsigned integer comparison

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0 as we can save 3 gas (id is uint so it will never be less than 0, so this check only needs to ensure that id is not equal to 0)

File: RubiconMarket.sol: line 983

    //find the id of the next higher offer after offers[id]
    function _find(uint256 id) internal view returns (uint256) {
        require(id > 0);

RubiconMarket.sol: line 837

File: RubiconMarket.sol: line 876

File: RubiconMarket.sol: line 985

File: BathPair.sol: line 515

File: BathPair.sol: line 597

File: BathPair.sol: line 611

File: BathBuddy.sol: line 102

No need to initialize variables with their defaults.

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address and so on). Explicitly initializing it with its default value is an anti-pattern and wastes gas as It costs more gas to initialize variables to zero than to let the default of zero be applied

RubiconMarket.sol: line 990

  uint256 old_top = 0;

File: BathPair.sol line 310

bool assigned = false;

Use short circuit

The operators “||” and “&&” apply the common short-circuiting rules. This means that in the expression “f(x) || g(y)”, if “f(x)” evaluates to true, “g(y)” will not be evaluated even if it may have side-effects. So setting less costly function to “f(x)” and setting costly function to “g(x)” is efficient.

File: rubiconmarket.sol line 32

    function isAuthorized(address src) internal view returns (bool) {
        if (src == address(this)) {
            return true;
        } else if (src == owner) {
            return true;
        } else {
            return false;
        }
    }
}

The above can be rewritten as follows , which would cost us around 14 gas less.

    function isAuthorized(address src) internal view returns (bool) {
        if (src == owner || src == address(this) ) {
            return true;
    
        } else {
            return false;
        }
    }

Commented code

File: RubiconMarket.sol line 93-110

// /// @notice ERC-20 interface as derived from EIP-20
// contract ERC20 {
//     function totalSupply() public view returns (uint256);


//     function balanceOf(address guy) public view returns (uint256);


//     function allowance(address src, address guy) public view returns (uint256);


//     function approve(address guy, uint256 wad) public returns (bool);


//     function transfer(address dst, uint256 wad) public returns (bool);


//     function transferFrom(
//         address src,
//         address dst,
//         uint256 wad
//     ) public returns (bool);
// }

This piece of code should be deleted as it serves no purpose

Emit memory variable not storage

File: RubiconMarket.sol line 747

    function setBuyEnabled(bool buyEnabled_) external auth returns (bool) {
        buyEnabled = buyEnabled_;
        emit LogBuyEnabled(buyEnabled);
        return true;
    }

We seem to be emmiting a storage variable rather than the cached one which is buyEnabled_

RubiconMarket.sol line 760

    function setMatchingEnabled(bool matchingEnabled_)
        external
        auth
        returns (bool)
    {
        matchingEnabled = matchingEnabled_;
        emit LogMatchingEnabled(matchingEnabled);
        return true;
    }

In the above, we should change the code to emit matchingEnabled_

Cache storage in memory to minimize number of SLOADs

File: Bathtoken.sol line 278

    /// @notice The function for a strategist to cancel an outstanding Market Offer
    function cancel(uint256 id, uint256 amt) external onlyPair {
        outstandingAmount = outstandingAmount.sub(amt);
        IRubiconMarket(RubiconMarketAddress).cancel(id);

        emit LogPoolCancel(
            id,
            IERC20(underlyingToken),
            amt,
            underlyingBalance(),
            outstandingAmount,
            totalSupply
        );
    }

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory Cache the outstandingAmount and totalSupply, don't work with storage values. State variables should be cached in stack variables rather than re-reading them from storage

Other instances

In the following instances, outstandingAmount and totalSupply should be cached as they are read several times.

File: Bathtoken.sol line 294

File: Bathtoken.sol line 306

File: Bathtoken.sol line 346

File: Bathtoken.sol line 557

Caching the length in for loops

Reading array length at each iteration of the loop takes 6 gas in the stack The solidity compiler will always read the length of the array during each iteration.

Bathpair.sol line 303

    /// @notice A function that returns the index of uid from array
    /// @dev uid must be in array for the purposes of this contract to enforce outstanding trades per strategist are tracked correctly
    function getIndexFromElement(uint256 uid, uint256[] storage array)
        internal
        view
        returns (uint256 _index)
    {
        bool assigned = false;
        for (uint256 index = 0; index < array.length; index++) {
            if (uid == array[index]) {
                _index = index;
                assigned = true;
                return _index;
            }
        }
        require(assigned, "Didnt Find that element in live list, cannot scrub");
    }

In the above case, the solidity compiler will always read the length of the array during each iteration and since the following is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929) for each iteration except for the first),

This extra costs can be avoided by caching the array length (in stack) as follows.

uint length = array.length;
for (uint i = 0; i < length; i++) {
    // do something that doesn't change arr.length
}

Other instances:

File: Bathtoken.sol line 627

 /// @notice Function to distibute non-underlyingToken Bath Token incentives to pool withdrawers
    /// @dev Note that bonusTokens adhere to the same feeTo and feeBPS pattern. Fees sit on BathBuddy to act as effectively accrued to the pool.
    function distributeBonusTokenRewards(
        address receiver,
        uint256 sharesWithdrawn,
        uint256 initialTotalSupply
    ) internal {
        if (bonusTokens.length > 0) {
            for (uint256 index = 0; index < bonusTokens.length; index++) {
                IERC20 token = IERC20(bonusTokens[index]);
                // Note: Shares already burned in Bath Token _withdraw

                // Pair each bonus token with a lightly adapted OZ Vesting wallet. Each time a user withdraws, they
                //  are released their relative share of this pool, of vested BathBuddy rewards
                // The BathBuddy pool should accrue ERC-20 rewards just like OZ VestingWallet and simply just release the withdrawer's relative share of releaseable() tokens
                if (rewardsVestingWallet != IBathBuddy(0)) {
                    rewardsVestingWallet.release(
                        (token),
                        receiver,
                        sharesWithdrawn,
                        initialTotalSupply,
                        feeBPS
                    );
                }
            }
        }
    }

A similar concept was implemented here: BathPair.sol line 478

Use calldata instead of memory

use calldata when you only need read-only data, avoiding the cost of allocating memory or storage. File: RubiconMarket.sol line 256

    function bump(bytes32 id_) external can_buy(uint256(id_)) {
        uint256 id = uint256(id_);
        emit LogBump(
            id_,
            keccak256(abi.encodePacked(offers[id].pay_gem, offers[id].buy_gem)),
            offers[id].owner,
            offers[id].pay_gem,
            offers[id].buy_gem,
            uint128(offers[id].pay_amt),
            uint128(offers[id].buy_amt),
            offers[id].timestamp
        );
    }

Bytes32 id_ should be stored in calldata to reduce gas cost

bytes32 calldata id_

Other Instances: Bathpair.sol line 577

    /// @notice Batch scrub outstanding strategist trades and return funds to LPs
    function scrubStrategistTrades(uint256[] memory ids)
        external
        onlyApprovedStrategist(msg.sender)
    {
        for (uint256 index = 0; index < ids.length; index++) {
            uint256 _id = ids[index];
            scrubStrategistTrade(_id);
        }
    }

File: RubiconMarket.sol line 431

File: RubiconMarket.sol line 590

File: RubiconMarket.sol line 594