Rubicon contest - _Adam's results

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

approve/safeApprove usage is discouraged

As per Openzepplin approve/safeApprove is discouraged in favor of using safeIncreaseAllowance.

RubiconRouter.sol#L157 RubiconRouter.sol#L465 BathToken.sol#L214 BathToken.sol#L256 BathHouse.sol#L165 BathHouse.sol#L180-L183

Wrap transfer/transferFrom in require statements.

If choosing to not use something like openZepplins safeTransfer/safeTransferFrom it is good to add require checks to ensure transfers/tranferFroms revert in cast of failures. This is already done for all transfers/transferFroms in RubiconMarket.sol/BathHouse.sol but is missing in the following locations.

BathPair.sol#L601 BathPair.sol#L615 BathToken.sol#L353-L357 BathToken.sol#L565 BathToken.sol#L602 BathToken.sol#L605 RubiconRouter.sol#L202 RubiconRouter.sol#L251 RubiconRouter.sol#L274 RubiconRouter.sol#L303 RubiconRouter.sol#L320 RubiconRouter.sol#L348 RubiconRouter.sol#L356 RubiconRouter.sol#L366 RubiconRouter.sol#L374 RubiconRouter.sol#L377 RubiconRouter.sol#L406 RubiconRouter.sol#L419 RubiconRouter.sol#L434 RubiconRouter.sol#L451 RubiconRouter.sol#L471 RubiconRouter.sol#L486 RubiconRouter.sol#L491 RubiconRouter.sol#L548

Two Step Procedures for Updating Owner/Admin

Ownership/Admin functions don’t protect against accidentally transferring to an incorrect address. Recommend setting up a two step process to transfer ownership.

RubiconMarket.sol#L22-L25 BathToken.sol#L250-L252 BathHouse.sol#L253-L255

Looping over storage variables

When using the length of storage variable in a for loop, each iteration will need to use an SLOAD (roughly 100 gas). If you cache the length into a local variable first each iteration will only need an MLOAD (roughly 3 gas).

BathToken.sol#L634-L635 Caching bonusTokens.length will also save another SLOAD as it is used in the if statement on line 634.

BathPair.sol#L311 In this case the variable array can be switched from storage to calldata as it is only ever referenced not modified.

For a small gas saving in all for loops you can also replace i++ with ++i, this will save roughly 3 gas per iteration.

Minimizing SLOADs

Whenever referencing a storage variable multiple times within a scope without making any changes it is cheaper to cache to the stack and use that. (3 gas per use vs 100 gas for SLOAD).

BathToken.sol#L557-L583 The uint256 totalSupply is called 3 times in _deposit().

BathToken.sol#L588-L617 The address feeTo is used 3 times in _withdraw().

BathPair.sol#L160-L190 The address bathHouse is used 4 times in enforceReserveRatio().

Error strings in require functions

As your using a solidity version < 0.8.4 you can’t use custom errors to reduce gas cost, but you can shorten error strings to less then 32 bytes.

RubiconMarket.sol#L306 RubiconMarket.sol#L310 RubiconMarket.sol#L571-L575 RubiconRouter.sol#L338 RubiconRouter.sol#L392 RubiconRouter.sol#L506 BathHouse.sol#L143-L150 BathHouse.sol#L162 BathHouse.sol#L177 BathHouse.sol#L399 BathHouse.sol#L411 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#L318 BathPair.sol#L572

Uints > 0 in require Statements

When checking whether a uint is greater then 0 within a require statement you can save a small amount of gas by replacing with != 0.

RubiconMarket.sol#L400 RubiconMarket.sol#L402 RubiconMarket.sol#L985 RubiconMarket.sol#L1002 BathHouse.sol#L111 BathHouse.sol#L281

No need to compare statement == true in a require function

When comparing a statement is equal to true in a require function you can remove the == true to save some gas. If the statement is false the require function will fail anyway.

BathToken.sol#L228 BathPair.sol#L149-L150