Reserve contest - RaymondFam's results

Typo mistakes

File: IMain.sol#L32

- *   - usually (but not always) contain sizeable state that require a proxy
+ *   - usually (but not always) contain sizable state that requires a proxy

File: Auth.sol#L22

-     * Typically freezing thaws on its own in a predetemined number of blocks.
+     * Typically freezing thaws on its own in a predetermined number of blocks.

File: Furnace.sol#L66

-    //   lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay leriod)
+    //   lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay period)

File: IAsset.sol#L92

-    /// @return The status of this collateral asset. (Is it defaulting? Might it soon?)
+    /// @return The status of this collateral asset. (Is it defaulting? Might it be soon?)

Modularity on import usages

For cleaner Solidity code in conjunction with the rule of modularity and modular programming, use named imports with curly braces instead of adopting the global import approach.

For instance, the import instances below could be refactored as follows:

File: Component.sol#L4-L9

- import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+ import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
- import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+ import {UUPSUpgradeable.sol} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
    [ ... ]

Interfaces and libraries should be separately saved and imported

Some contracts have interface(s) or library showing up in its/their entirety at the top/bottom of the contract facilitating an ease of references on the same file page. This has, in certain instances, made the already large contract size to house an excessive code base. Additionally, it might create difficulty locating them when attempting to cross reference the specific interfaces embedded elsewhere but not saved into a particular .sol file.

Consider saving the interfaces and libraries entailed respectively, and having them imported like all other files.

Here are some of the instances entailed:

File: IAsset.sol#L75 File: IMain.sol File: BasketHandler.sol#L61-L104 File: ATokenFiatCollateral.sol#L10-L27

Inconsistency in interface naming

Some interfaces in the code bases are named without the prefix I that could cause confusion to developers and readers referencing or interacting with the protocol. Consider conforming to Solidity's naming conventions by having the specific instance below refactored as follows:

File: IMain.sol#L166

- interface TestIMain is IMain {
+ interface ITestIMain is IMain {

Contracts should have their names exactly matched to their filenames

Contracts, interfaces and libraries with their names differing with the filenames could sometimes create confusion and difficulty in cross referencing.

For instance, the abstract contract below has been named ComponetP1 where as the file has been saved as Component.sol. Consider sticking to the same name where possible:

File: Component.sol#L14

abstract contract ComponentP1 is

Camel/snake casing function names

Function names should also be conventionally camel cased where possible. If snake case is preferred/adopted, consider lower casing them.

Here is one of the numerous instances entailed:

File: ATokenFiatCollateral.sol#L26

    function REWARD_TOKEN() external view returns (IERC20);

require() over assert()

As documented by Solidity Documentations:

"Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic."

The protocol uses assert() in multiple places of the code bases. Despite the strip of gas saving benefits for Solidity version ^0.8.0, require() should still be used for checking error conditions on inputs and return values.

Here are some of the instances entailed:

File: RecollateralizationLib.sol#L110

        assert(doTrade); // @ audit Associated with the return value, `doTrade`

File: TradeLib.sol

            // @ audit Associated with the input parameter, `trade`
44:        assert(trade.buyPrice > 0 && trade.buyPrice < FIX_MAX && trade.sellPrice < FIX_MAX); 

            // @ audit Associated with the input parameter, `trade`
108:        assert( 
109:            trade.sellPrice > 0 &&
110:                trade.sellPrice < FIX_MAX && 
111:                trade.buyPrice > 0 &&
112:                trade.buyPrice < FIX_MAX
113:        );

168:            assert(errorCode == 0x11 || errorCode == 0x12); // @ audit Associated with try catch

170:            assert(keccak256(reason) == UIntOutofBoundsHash); // audit Associated with try catch

Descriptive and Verbose Options

Consider making the naming of local variables more verbose and descriptive so all other peer developers would better be able to comprehend the intended statement logic, significantly enhancing the code readability.

Here are some of the instances entailed:

File: TradeLib.sol

55:         uint192 s = trade.sellAmount > maxSell ? maxSell : trade.sellAmount; // {sellTok} @ audit s

59:        uint192 b = safeMulDivCeil( // @ audit b

Time Units

According to:

https://docs.soliditylang.org/en/v0.8.14/units-and-global-variables.html

suffixes like seconds, minutes, hours, days and weeks after literal numbers can be used to specify units of time where seconds are the base unit and units are considered naively in the following way:

1 == 1 seconds 1 minutes == 60 seconds 1 hours == 60 minutes 1 days == 24 hours 1 weeks == 7 days

As an example, to minimize human error while making the assignment more verbose, the constant declarations below may respectively be rewritten as follows:

File: BackingManager.sol#L33

-    uint48 public constant MAX_TRADING_DELAY = 31536000; // {s} 1 year
+    uint48 public constant MAX_TRADING_DELAY = 365 days; // {s} 1 year

File: Broker.sol#L24

-    uint48 public constant MAX_AUCTION_LENGTH = 604800; // {s} max valid duration - 1 week
+    uint48 public constant MAX_AUCTION_LENGTH = 1 weeks; // {s} max valid duration - 1 week

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the mapping instance below may be refactored as follows:

File: AssetRegistry.sol#L93

-        assets[asset.erc20()] = IAsset(address(0));
+        delete assets[asset.erc20()];

##Minimization of truncation The number of divisions in an equation should be reduced to minimize truncation frequency, serving to achieve higher precision. And, where deemed fit, comment the code line with the original multiple division arithmetic operation for clarity reason.

For instance, the equation below with three division may be refactored as follows:

File: StRSR.sol#L427

-        return (FIX_SCALE_SQ + (stakeRate / 2)) / stakeRate; // ROUND method
+        return ((FIX_SCALE_SQ * 2) + stakeRate) / (stakeRate * 2); // ROUND method

Events associated with setter functions

Consider having events associated with setter functions emit both the new and old values instead of just the new value.

Here are some of the instances entailed:

File: StRSR.sol#L803-L809

    function setName(string calldata name_) external governance {
        name = name_;
    }

    function setSymbol(string calldata symbol_) external governance {
        symbol = symbol_;
    }

Modifier on repeated code

Code line repeatedly used may be grouped into a modifier.

For instance, a modifier for the identical require statement in setUnstakingDelay() and setRewardPeriod() of StRSR.sol may be refactored as follows:

File: StRSR.sol#L812-L825

+    modifier ratioCompatible() {
+        _;
+        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
+    }

-    function setUnstakingDelay(uint48 val) public governance {
+    function setUnstakingDelay(uint48 val) public ratioCompatible governance {
        require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
        emit UnstakingDelaySet(unstakingDelay, val);
        unstakingDelay = val;
-        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
    }

    /// @custom:governance
-    function setRewardPeriod(uint48 val) public governance {
+    function setRewardPeriod(uint48 val) public ratioCompatible governance {
        require(val > 0 && val <= MAX_REWARD_PERIOD, "invalid rewardPeriod");
        emit RewardPeriodSet(rewardPeriod, val);
        rewardPeriod = val;
-        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
    }

Missing check on negative price

price() in OracleLib.sol should have a check for negative price. The revert will also prevent an underflow in the return statement when casting a negative integer to uint256().

File: OracleLib.sol#L14-L31

+    error NegativePrice();    

    function price(AggregatorV3Interface chainlinkFeed, uint48 timeout)
        internal
        view
        returns (uint192)
    {
        (uint80 roundId, int256 p, , uint256 updateTime, uint80 answeredInRound) = chainlinkFeed
            .latestRoundData();

        if (updateTime == 0 || answeredInRound < roundId) {
            revert StalePrice();
        }
        // Downcast is safe: uint256(-) reverts on underflow; block.timestamp assumed < 2^48
        uint48 secondsSince = uint48(block.timestamp - updateTime);
        if (secondsSince > timeout) revert StalePrice();

+        if (p < 0) revert NegativePrice(); //

        // {UoA/tok}
        return shiftl_toFix(uint256(p), -int8(chainlinkFeed.decimals()));
    }

Workaround for solhint-disable-next-line no-empty-blocks

The constructor in Main.sol can be rewritten as follows to silence any warning flag without the need for the commented "solhint-disable-next-line no-empty-blocks".

Note: This feature is readily incorporated in the Solidity Wizard since the UUPS vulnerability discovery. You would just need to check UPGRADEABILITY to have the following constructor code block added to the contract:

File: Main.sol#L21-L23

    /// @custom:oz-upgrades-unsafe-allow constructor
-    // solhint-disable-next-line no-empty-blocks
-    constructor() initializer {}
+    constructor() {
+        _disableInitializers();
+    }

Inappropriate comments

Consider rewriting the first line of comments on freezeForever() as follows:

Note: permanent means lasting or intended to last or remain unchanged indefinitely. But quite the opposite, the permanent freeze can always be thawed via unfreeze() and refrozen via freeUntil().

File: Auth.sol#L143-L150

-    /// Enter a permanent freeze
+    /// Enter an indefinite freeze
    // checks:
    // - caller has the OWNER role
    // - unfreezeAt != type(uint48).max
    // effects: unfreezeAt' = type(uint48).max
    function freezeForever() external onlyRole(OWNER) {
        freezeUntil(MAX_UNFREEZE_AT);
    }

Unneeded comment

Underflow due to decrementing longFreezes[_msgSender()] in freezeLong() of Auth.sol will never happen because _msgSender() has already got its LONG-FREEZER role revoked when longFreezes[_msgSender()] == 0. This makes it impossible to reach the first line of the function logic.

File: Auth.sol#L135-L141

    function freezeLong() external onlyRole(LONG_FREEZER) {
-        longFreezes[_msgSender()] -= 1; // reverts on underflow
+        longFreezes[_msgSender()] -= 1; // No underflow would transpire. Apply unchecked { ... } if need be.

        // Revoke on 0 charges as a cleanup step
        if (longFreezes[_msgSender()] == 0) _revokeRole(LONG_FREEZER, _msgSender());
        freezeUntil(uint48(block.timestamp) + longFreeze);
    }

Inadequate boundary check

longFreeze should not only be greater than zero but also greater than MAX_SHORT_FREEZE or shortfreeze to be feasibly practical. Otherwise, it could likely end up having shortFreeze > longFreeze, making freezeLong() to readily revert (due to newUnfreezeAt < unfreezeAt) if freezeShort() had been invoked not too long ago.

Consider having setLongFreeze() refactored as follows:

    function setLongFreeze(uint48 longFreeze_) public onlyRole(OWNER) {
-        require(longFreeze_ > 0 && longFreeze_ <= MAX_LONG_FREEZE, "long freeze out of range");
+        require(longFreeze_ > shortFreeze && longFreeze_ <= MAX_LONG_FREEZE, "long freeze out of range");
        emit LongFreezeDurationSet(longFreeze, longFreeze_);
        longFreeze = longFreeze_;
    }

Better yet, set a reasonable threshold so that longFreeze_ > shortFreeze + threshold to further minimize edge cases.

Empty Event

The following event has no parameter in it to emit anything. Although the standard global variables like block.number and block.timestamp will implicitly be tagged along, consider adding some relevant parameters to the make the best out of this emitted event for better support of off-chain logging API.

File: IMain.sol#L154

    event MainInitialized();

File: Main.sol#L38

        emit MainInitialized();

Useful sort function

In manageTokensSortedOrder() of BackingManager.sol, erc20s must already be in sorted ascending order before it can call ArrayLib.sortedAndAllUnique(erc20s).

Here is a useful and quick sortAscending() that may be added to Array.sol on top of the off chain method the protocol might already adopt. (Note: IERC20[] memory arr will have to be cast into uint160[] memory arr prior to calling this function.)

    function sortAscending(
        uint160[] memory arr,
        int160 left,
        int160 right
    ) internal pure {
        int160 i = left;
        int160 j = right;
        if (i == j) return;
        uint160 pivot = arr[uint160(left + (right - left) / 2)];
        while (i <= j) {
            while (arr[uint160(i)] < pivot) i++;
            while (pivot < arr[uint256(j)]) j--;
            if (i <= j) {
                (arr[uint160(i)], arr[uint160(j)]) = (
                    arr[uint160(j)],
                    arr[uint160(i)]
                );
                i++;
                j--;
            }
        }
        if (left < j) _quickSort(arr, left, j);
        if (i < right) _quickSort(arr, i, right);
    }

allUnique() in Array.sol could have a remove logic embedded

Consider having allUnique() refactored such that it will remove one of the two identical elements, e.g. replace the would be removed element with the last element and then calling arr.pop().

Alternatively, here is another useful removePosition() that can be invoked without affecting the order of the elements in the array:

  function _removePosition(IERC20[] memory arr, uint8 position)
    internal
    returns (IERC20[] memory newArr)
  {
    uint256 length = arr.length;
    require(position < length);
    newArr = new arr[](length - 1);
    uint256 i;
    for (; i < position; ) {
      newArr[i] = arr[i];
      unchecked {
        ++i;
      }
    }
    for (; i < length - 1; ) {
      unchecked {
        newArr[i] = arr[i + 1];
        ++i;
      }
    }
  }

FLOOR over CEIL

In openTrade() of Broker.sol, CEIL is the input RoundMode in mulDiv256() to return result++ to req.sellAmount.

This could still end up having req.sellAmount > GNOSIS_MAX_TOKENS if maxQty == req.sellAmount. For this reason, consider refactoring the affected code by replacing CEIL with FLOOR to avoid the incidental edge cases:

File: Broker.sol#L104-L107

        if (maxQty > GNOSIS_MAX_TOKENS) {
-            req.sellAmount = mulDiv256(req.sellAmount, GNOSIS_MAX_TOKENS, maxQty, CEIL);
+            req.sellAmount = mulDiv256(req.sellAmount, GNOSIS_MAX_TOKENS, maxQty, FLOOR);
            req.minBuyAmount = mulDiv256(req.minBuyAmount, GNOSIS_MAX_TOKENS, maxQty, FLOOR);
        }

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: Component.sol#L41-L44

+    function _notPausedOrFrozen() private view {
+        require(!main.pausedOrFrozen(), "paused or frozen");
+    }

    modifier notPausedOrFrozen() {
-        require(!main.pausedOrFrozen(), "paused or frozen");
+        _notPausedOrFrozen();
        _;
    }

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

Here are some of the instances entailed:

File: RecollateralizationLib.sol

70:        ComponentCache memory components = ComponentCache({

78:        TradingRules memory rules = TradingRules({

83:        Registry memory reg = components.reg.getRegistry();

              [ ... ]

Merging of if blocks

In price() of OracleLib.sol, the two if blocks can be merged as follows to save gas both on contract size and function calls:

File: OracleLib.sol#L14-L31

    function price(AggregatorV3Interface chainlinkFeed, uint48 timeout)
        internal
        view
        returns (uint192)
    {
        (uint80 roundId, int256 p, , uint256 updateTime, uint80 answeredInRound) = chainlinkFeed
            .latestRoundData();

-        if (updateTime == 0 || answeredInRound < roundId) {
+        if (uint48(block.timestamp - updateTime) > timeOut || answeredInRound < roundId) {
            revert StalePrice();
        }
        // Downcast is safe: uint256(-) reverts on underflow; block.timestamp assumed < 2^48
-        uint48 secondsSince = uint48(block.timestamp - updateTime);
-        if (secondsSince > timeout) revert StalePrice();

        // {UoA/tok}
        return shiftl_toFix(uint256(p), -int8(chainlinkFeed.decimals()));
    }

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the instance below may be refactored as follows:

File: RecollateralizationLib.sol#L401

-        if (address(trade.sell) == address(0) && address(trade.buy) != address(0)) {
+        if (!(address(trade.sell) != address(0) || address(trade.buy) == address(0))) {

Early if block

In melt() of Furnace.sol, the last if block should be executed before lastPayoutBal is updated. In the event amount == 0 due to ratio == 0, the function could end with a return and save some gas.

File: Furnace.sol#L70-L84

    function melt() external notPausedOrFrozen {
        if (uint48(block.timestamp) < uint64(lastPayout) + period) return;

        // # of whole periods that have passed since lastPayout
        uint48 numPeriods = uint48((block.timestamp) - lastPayout) / period;

        // Paying out the ratio r, N times, equals paying out the ratio (1 - (1-r)^N) 1 time.
        uint192 payoutRatio = FIX_ONE.minus(FIX_ONE.minus(ratio).powu(numPeriods));

        uint256 amount = payoutRatio.mulu_toUint(lastPayoutBal);

        lastPayout += numPeriods * period;

+        if (amount == 0) {
+            return;
+        else {
+            rToken.melt(amount);
+        }

        lastPayoutBal = rToken.balanceOf(address(this)) - amount;
-        if (amount > 0) rToken.melt(amount);
    }

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ++i ;} to use the previous wrapping behavior further saves gas just as in the for loop below as an example:

File: RecollateralizationLib.sol#L437-L454

-        for (uint256 i = 0; i < len; ++i) {
+        for (uint256 i = 0; i < len;) {
              ICollateral coll = components.reg.toColl(basketERC20s[i]);

              [ ... ]

+            unchecked {
+                ++i;
+            }
          }

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, the following >= inequality instance may be refactored as follows:

File: RewardableLib.sol#L78

-                assert(erc20s[i].balanceOf(address(this)) >= liabilities[erc20s[i]]);
+                assert(erc20s[i].balanceOf(address(this)) > liabilities[erc20s[i]] - 1);

break or continue in for loop

for loop entailing large array with reverting logic should incorporate break or continue to cater for element(s) failing to get through the iteration(s). This will tremendously save gas on instances where the loop specifically fails to execute at the end of the iterations.

Here is an instance entailed where refresh() is associated with try catch reverting code:

File: AssetRegistry.sol#L46-L52

    function refresh() public {
        // It's a waste of gas to require notPausedOrFrozen because assets can be updated directly
        uint256 length = _erc20s.length();
        for (uint256 i = 0; i < length; ++i) {
            assets[IERC20(_erc20s.at(i))].refresh();
        }
    }

Split require statements using &&

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

Here is a sample instance entailed:

File: Broker.sol#L134-L137

        require(
            newAuctionLength > 0 && newAuctionLength <= MAX_AUCTION_LENGTH,
            "invalid auctionLength"
        );

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: Deployer.sol#L102-L249

    function deploy(
        string memory name,
        string memory symbol,
        string calldata mandate,
        address owner,
        DeploymentParams memory params
-    ) external returns (address) {
+    ) external returns (address rTokenAddr) {

        [ ... ]

-        return (address(components.rToken));
+        rTokenAddr = address(components.rToken);
    }

Unneeded logic

In Auth.sol, a LONG_FREEZER role is given 6 charges whereas the SHORT_FREEZER role is only entitled to a one time use. And when they are run out of the assigned privileges, they can/will be recharged via grantRole(). In my opinion, the logic entailed is unnecessary since these assignees are trusted partners and/or associates to the protocol. If need be, the owner can always revoke their roles via the inherited revokeRole().

Consider removing the following lines of codes to save gas both on contract size and function calls:

File: Auth.sol

- 7: uint256 constant LONG_FREEZE_CHARGES = 6; // 6 uses

- 121:        // Revoke short freezer role after one use
- 122:        _revokeRole(SHORT_FREEZER, _msgSender());

- 138:        // Revoke on 0 charges as a cleanup step
- 139:        if (longFreezes[_msgSender()] == 0) _revokeRole(LONG_FREEZER, _msgSender());

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For instance, the += instance below may be refactored as follows:

File: RToken.sol#L330

-            liabilities[IERC20(erc20s[i])] += deposits[i];
+            liabilities[IERC20(erc20s[i])] = liabilities[IERC20(erc20s[i])] + deposits[i];

Ternary over if ... else

Using ternary operator instead of the if else statement saves gas.

For instance, the code block below may be refactored as follows:

File: RToken.sol#L392-L396

 earliest
     ? refundSpan(account, queue.left, endId)
     : refundSpan(account, endId, queue.right);

Local variable not fully utilized

The input parameter, val, in setUnstakingDelay() and setRewardPeriod() of StRSR.sol could have been used in the second require statement to separately save 1 SLOAD on unstakingDelay:

File: StRSR.sol#L812-L825

    function setUnstakingDelay(uint48 val) public governance {
        require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
        emit UnstakingDelaySet(unstakingDelay, val);
        unstakingDelay = val;
-        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
+        require(rewardPeriod * 2 <= val, "unstakingDelay/rewardPeriod incompatible");
    }

    /// @custom:governance
    function setRewardPeriod(uint48 val) public governance {
        require(val > 0 && val <= MAX_REWARD_PERIOD, "invalid rewardPeriod");
        emit RewardPeriodSet(rewardPeriod, val);
        rewardPeriod = val;
-        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
+        require(val * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
    }

Early checks

Checks in a function logic should be as early as possible to minimize gas waste in the event of a revert.

For instance, the same instances in the preceding issue with the input parameter, val, fully utilized should have been refactored as follows:

File: StRSR.sol#L812-L825

    function setUnstakingDelay(uint48 val) public governance {
        require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
+        require(rewardPeriod * 2 <= val, "unstakingDelay/rewardPeriod incompatible");
        emit UnstakingDelaySet(unstakingDelay, val);
        unstakingDelay = val;
-        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
    }

    /// @custom:governance
    function setRewardPeriod(uint48 val) public governance {
        require(val > 0 && val <= MAX_REWARD_PERIOD, "invalid rewardPeriod");
+        require(val * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
        emit RewardPeriodSet(rewardPeriod, val);
        rewardPeriod = val;
-        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
    }

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the function below may be refactored as follows:

File: BackingManager.sol#L256

-    function setTradingDelay(uint48 val) public governance {
+    function setTradingDelay(uint48 val) public payable governance {

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.9",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Use assembly to check for address(0)

Using assembly to check for address(0) saves 6 gas for each instance.

As an example, the following instance may be refactored as follows:

File: ComponentRegistry.sol#L36-L40

    function _setRToken(IRToken val) private {
-        require(address(val) != address(0), "invalid RToken address");

+        assembly {
+            if iszero(address(val)) {
+                mstore(0x00, "invalid RToken address")
+                revert(0x00, 0x20)
+            }

        emit RTokenSet(rToken, val);
        rToken = val;
    }

Unneeded if block

In BackingManager.sol, the second check in the if block of handoutExcessAssets() is unneeded because basketHandler.fullyCollateralized() has already confirmed that basketsHeldBy(address(backingManager)) >= rToken.basketsNeeded() before handoutExcessAssets() is privately invoked:

File: BasketHandler.sol#L273-L275

    function fullyCollateralized() external view returns (bool) {
        return basketsHeldBy(address(backingManager)) >= rToken.basketsNeeded();
    }

As such consider refactoring the following code line to save gas on contract size and function calls:

File: BackingManager.sol#L192-L206

-            if (held.gt(needed)) {
                // gas-optimization: RToken is known to have 18 decimals, the same as FixLib
                uint192 totalSupply = _safeWrap(rToken.totalSupply()); // {rTok}

                // {BU} = {BU} - {BU}
                uint192 extraBUs = held.minus(needed);

                // {rTok} = {BU} * {rTok / BU} (if needed == 0, conv rate is 1 rTok/BU)
                uint192 rTok = (needed > 0) ? extraBUs.mulDiv(totalSupply, needed) : extraBUs;

                // gas-optimization: RToken is known to have 18 decimals, same as FixLib
                rToken.mint(address(this), uint256(rTok));
                rToken.setBasketsNeeded(held);
                needed = held;
-            }

The first condition check is unneeded

In quantityMulPrice() of BasketHandler.sol, the comment before the first if block already mentioned qty will never = 0 here because of the check in _price(). For this reason, qty == 0 is always going to return false, making this check a waste of gas.

Consider refactoring the affected code line as follows:

File: BasketHandler.sol#L358-L359

        //      qty will never = 0 here because of the check in _price()
-        if (qty == 0 || p == 0) return 0;
+        if (p == 0) return 0;

Unneeded assert()

In init() of GnosisTrade.sol, asserting origin_ != address(0) is unnecessary because it is the caller, _msgSender(), that has invoked openTrade() in Broker.sol before trade.init() is externally called.

Consider removing it to save gas both on contract size and function calls:

File: GnosisTrade.sol#L98

-        assert(origin_ != address(0));