Rubicon contest - Funen's results

LOW ISSUE RISK

1. Title : using pragma abicoder V2

Since BathPair.sol was using pragma experimental ABIEncoderV2 and used pragma ^0.7.6, it because the ABI coder v2 is not considered experimental anymore, it can be selected via pragma abicoder v2 instead since Solidity 0.7.4.

Tools

Manual Review

Recommended Mitigation

you can change it into `pragma abicoder v2;

2. Title : Actual code can be changed for good reason

since function _offeru() can be changed as _offer since it can be used as it should be for good readibility and code.

Tool Used

Manual Review

3. Title : Use a more recent version of solidity

Use a solidity version of at least 0.8.0 to get overflow protection without needing SafeMath

##Occurances

1.) File: main/contracts/RubiconMarket.sol

pragma solidity =0.7.6;

2.) File: main/contracts/RubiconRouter.sol Non Critical

pragma solidity =0.7.6;

3.) File : main/contracts/rubiconPools/BathToken.sol

pragma solidity =0.7.6;

4.) File : main/contracts/rubiconPools/BathPair.sol

pragma solidity =0.7.6;

5.) File : main/contracts/rubiconPools/BathHouse.sol

pragma solidity =0.7.6;

4. Title : Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

1.) File : contracts/RubiconMarket.sol (Line 643)

//match "close enough" orders?

2.) File : contracts/RubiconRouter.sol (Line.243)

//naively assume no fill_amt here for loop purposes?

3.) File : contracts/rubiconPools/BathHouse.sol (Line 318)

/// @dev required in case the market address ever changes.. #battleScars

4.) File : contracts/rubiconPools/BathHouse.sol (Line.415)

the Bath House admin via 

changed to

the Bath House via admin

5.) File : contracts/rubiconPools/BathPair.sol (Line.229)

        // if real

6.) File : contracts/rubiconPools/BathPair.sol (Line.234)

       // otherwise didn't fill so cancel

7.) File : contracts/RubiconRouter.sol (Line.187)

      // Return the wouldbe resulting swap amount => remove `the`

5. Title : Unmatch comment with actual code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/ERC20.sol#L46-L54

its said that : ``` * All three of these values are immutable``

but in actual codename and symbol arent immutable value. so it would be missleading information on comment section.

##Tool Used Manual Review, Remix

##Recommended Mitigation Change it or remove it

NON CRITICAL ISSUE

6. Title : Typo Comment

This typo can be changed

Recommended Mitigation

1.) File : contracts/rubiconPools/BathToken.sol (Line.627)

`distibute` can be changed into `distribute`

2.) File : contracts/rubiconPools/BathPair.sol (Line.245)

`pas amount too` can be changed into `pass amount too`

3.) contracts/rubiconPools/BathPair.sol (Line.131)

`DEprecate` can be changed into `Deprecate`

7. Title : Multiple address mappings can be combined into a single mapping of an address to a struct

8. File : main/contracts/rubiconPools/BathHouse.sol (line 56)

    mapping(address => address) public tokenToBathToken;

8. Title : NatSpec Upgrade

This can be good for readibility

1.) File : main/contracts/rubiconPools/BathHouse.sol (Lines 131-140)

changed @param initialLiquidityNew , @param desiredPairedAsset to above and give an information

2.) File : main/contracts/RubiconRouter.sol (Lines 128)

missing @return

3.) File : main/contracts/RubiconRouter.sol (Lines 160)

missing @return

4.) File : contracts/RubiconRouter.sol (Lines 192-L193)

changed @dev into @param pay_amt and @param buy_amt_min

5.) File : contracts/RubiconRouter.sol (Lines 217)

add @dev , changed above @param route , @param to , and missing @param pay_amt, @param buy_amt_min , @param expectedMarketFeeBPS , @return

6.) File : contracts/RubiconRouter.sol Line 266)

missing @return

7.) File : contracts/RubiconRouter.sol (Line. 383)

move information above and adding information like @dev, @params and @return

8.) File : main/contracts/RubiconMarket.sol (Line.612)

move information above and adding @dev, @params and @return

9.) File : main/contracts/RubiconMarket.sol (Line.627)

move information above and adding @dev, @params , and @return

10.) File : main/contracts/RubiconMarket.sol (Line.637)

move information above and adding @dev, @params , and @return

11.) File : main/contracts/RubiconMarket.sol (Line.1049)

move information above and adding @dev, @params , and @return

12.) File : contracts/rubiconPools/BathPair.sol (Line.440)

move information above and adding @dev, @params.

13.) File : contracts/rubiconPools/BathPair.sol (Line.535)

move information above and adding @dev, @params.

9. Title : Unused Comment

This comment was unnecessary it can be deleted instead or remain the same.

1.) File : contracts/RubiconRouter.sol (Line.547)

        // msg.sender.transfer(fill);

2.) File : contracts/RubiconRouter.sol (Line.119)

       // bids[index] = nextBestBid;

3.) File : contracts/rubiconPools/BathPair.sol (Line.224)

      //ask

4.) File : contracts/rubiconPools/BathPair.sol (Line.225)

      //bid

10. Title : Unused local variable for remove warning.

##POC

https://ethereum.stackexchange.com/questions/38420/compile-error-warning-unused-local-variable

EVM takes gas for execute your (op)code. If you write one line extra it will EVM will consume some gas to execute that statements. And you need to be very careful while creating variables, because it will be cost gas based on data type. Solidity compiler is giving warring for unused variables to delete. Because when you execute contract method it will cost. In near feature cost of the gas will be more. So please delete unused variables so it will reduce final opcodes i.e gas consumption of your method will be low.

File : main/contracts/rubiconPools/BathToken.sol#L184 ([1]) (https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L184)

    address _feeTo

File : main/contracts/rubiconPools/BathToken.sol#L416 ([2]) (https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L416)

    function maxDeposit(address receiver) {

File : main/contracts/rubiconPools/BathToken.sol#L450 ([3]) (https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L450)

    function maxMint(address receiver) public pure returns (uint256 maxShares) {

1. Activate enable optimization for saving more gas

Since RubiconMarket.sol contract code size exceeds 24576 bytes (a limit introduced in Spurious Dragon). This contract may not be deployable on mainnet. Consider enabling the optimizer (with a low "runs" value!), turning off revert strings, or using libraries.

##Tool Used Remix, VSC

##Recommended Mitigation enabling optimizer (ovm)

2. Title : Change 2**256 - 1 to type(uint).max

in this condition if the deployment will be using new compiler ^0.8.0 , 2**256-1, its cheaper to using type(uint).max instead of using 2**256-1 calculation for unlimited approval. So it could be changed as it can be.

##Tool Used Manual Review

3. Title : Using short reason string can be used for saving more gas

Every reason string takes at least 32 bytes. Use short reason strings that fits in 32 bytes or it will become more expensive.

Tool Used

Remix, Manual Review

Occurances

contracts/RubiconMarket.sol#L299           "Insufficient funds to cover fee"
contracts/RubiconMarket.sol#L306           "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee"
contracts/RubiconMarket.sol#L310           "_offer.pay_gem.transfer(msg.sender, quantity) failed"
contracts/RubiconMarket.sol#L571           "Offer was deleted or taken, or never existed."
contracts/RubiconMarket.sol#L574           "Offer can not be cancelled because user is not owner, and market is open, and offer sells required amount of tokens."
contracts/RubiconRouter.sol#L500           "Initial value in path not WETH"
contracts/RubiconRouter.sol#L506             "must send enough native ETH to pay as weth and account for fee"
contracts/rubiconPools/BathHouse.sol#L145             "bathToken already exists for that ERC20"
contracts/rubiconPools/BathHouse.sol#L149            "bathToken does not exist for that desiredPairedAsset"
contracts/rubiconPools/BathHouse.sol#L162             "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
contracts/rubiconPools/BathHouse.sol#L177             "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
contracts/rubiconPools/BathHouse.sol#L399            "Cant create bathToken for zero address"
contracts/rubiconPools/BathHouse.sol#L411            "no implementation set for bathTokens"
contracts/rubiconPools/BathPair.sol#L180                "Failed to meet asset pool reserve ratio"
contracts/rubiconPools/BathPair.sol#L188                "Failed to meet quote pool reserve ratio"
contracts/rubiconPools/BathPair.sol#L318                "Didnt Find that element in live list, cannot scrub"
contracts/rubiconPools/BathPair.sol#L572                "you are not the strategist that made this order"
contracts/peripheral_contracts/BathBuddy.sol#L45   "VestingWallet: beneficiary is zero address"
contracts/peripheral_contracts/BathBuddy.sol#L96   "Caller is not the Bath Token beneficiary of these rewards"

4. Title : Using ++i than i++ for saving more gas

Using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++.

Tools Used

Manual Review

Occurances

contracts/RubiconRouter.sol#L85           for (uint256 index = 0; index < topNOrders; index++)
contracts/RubiconRouter.sol#L169         for (uint256 i = 0; i < route.length - 1; i++)
contracts/RubiconRouter.sol#L227         for (uint256 i = 0; i < route.length - 1; i++)
contracts/rubiconPools/BathPair.sol#L311         for (uint256 index = 0; index < array.length; index++)
contracts/rubiconPools/BathPair.sol#L427         for (uint256 index = 0; index < quantity; index++)
contracts/rubiconPools/BathPair.sol#L480         for (uint256 index = 0; index < quantity; index++)
contracts/rubiconPools/BathPair.sol#L582        for (uint256 index = 0; index < ids.length; index++)
contracts/rubiconPools/BathToken.sol#L635     for (uint256 index = 0; index < bonusTokens.length; index++) 

5. Title : change uint256 i = 0 into uint256 i for saving more gas

using this implementation can saving more gas for each loops.

Tool Used

Manual Review

Recommended Mitigation

Change it without = 0

Occurances

contracts/RubiconRouter.sol#L85           for (uint256 index = 0; index < topNOrders; index++)
contracts/RubiconRouter.sol#L169         for (uint256 i = 0; i < route.length - 1; i++)
contracts/RubiconRouter.sol#L227         for (uint256 i = 0; i < route.length - 1; i++)
contracts/rubiconPools/BathPair.sol#L311         for (uint256 index = 0; index < array.length; index++)
contracts/rubiconPools/BathPair.sol#L427         for (uint256 index = 0; index < quantity; index++)
contracts/rubiconPools/BathPair.sol#L480         for (uint256 index = 0; index < quantity; index++)
contracts/rubiconPools/BathPair.sol#L582        for (uint256 index = 0; index < ids.length; index++)
contracts/rubiconPools/BathToken.sol#L635     for (uint256 index = 0; index < bonusTokens.length; index++) 

6. Title : betterway to used last_offer_id++ for saving more gas

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

this can be changed into prefix than postfix for saving more gas

Tool Used

Remix, Manual Review

Occurances

it can be used for this code too :

contracts/rubiconPools/BathPair.sol#L206         last_stratTrade_id++;

7. Title : Saving gas by removing = 0

This implementation code can be saving more gas by removing = 0, it because If a variable was not set/initialized, it is assumed to have default value to 0

Tool Used

Remix, VCS

Mitigation Step

Remove = 0

Occurances

contracts/RubiconRouter.sol#L82        uint256 lastBid = 0;
contracts/RubiconRouter.sol#L83        uint256 lastAsk = 0;
contracts/RubiconRouter.sol#L226      uint256 currentAmount = 0;
contracts/RubiconMarket.sol#L990     uint256 old_top = 0;

8. Title : Set value to constant for saving more gas

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

feeBPS can be set as constant

9. Title : Using operator || is more cost less gas than &&

This implementation can be set down below for saving more gas

Tool Used

Remix, VSC

POC

{
        require(
            askNumerators.length == askDenominators.length ||
                askDenominators.length == bidNumerators.length ||
                bidNumerators.length == bidDenominators.length ||
                ids.length == askNumerators.length,
            "not all input lengths match"
        );
        // 4955769 after changed operator `||`
        // 4956514 before changed operator `&&`