Reserve contest - shark's results

0. Use time units for better readability

Solidity has time units (seconds, minutes, hours, etc). As such, consider using time units instead of using literal numbers, this will increase readability and decrease the likelihood of mistakes being made.

For example:

File: Furnace.sol Line 16

    uint48 public constant MAX_PERIOD = 31536000; // {s} 1 year

Instead of the above (31536000), consider refactoring to:

    uint48 public constant MAX_PERIOD = 1 year;

Other instances found:

• File: Auth.sol Line 9
• File: Auth.sol Line 10
• File: BackingManager.sol Line 33
• File: StRSR.sol Line 37
• File: StRSR.sol Line 38
• File: Broker.sol Line 24

1. Use delete to clear variables instead of zero assignment

Using the delete keyword more clearly states your intention.

For instance:

File: RToken.sol Line 693-694

692:    // empty entire queue
693:    queue.left = 0;
694:    queue.right = 0;

Instead of above, consider using the delete keyword instead:

692:    // empty entire queue
693:    delete queue.left;
694:    delete queue.right;

Instances also found:

• File: RToken.sol Line 784
• File: RToken.sol Line 785
• File: StRSR.sol Line 575
• File: StRSR.sol Line 576
• File: StRSR.sol Line 587-588

2. Misleading comment

File: Auth.sol Line 135-141

135:    function freezeLong() external onlyRole(LONG_FREEZER) {
136:        longFreezes[_msgSender()] -= 1; // reverts on underflow
137:
138:        // Revoke on 0 charges as a cleanup step
139:        if (longFreezes[_msgSender()] == 0) _revokeRole(LONG_FREEZER, _msgSender());
140:        freezeUntil(uint48(block.timestamp) + longFreeze);
141:    }

In the code above, there is a comment stating that line 136 will "revert on underflow". However, it will never actually underflow. This is due to the LONG_FREEZER role getting revoked on 0 charges at line 139. And, since the LONG_FREEZER role is revoked at 0 charges, it no longer has the necessary role to call freezeLong(), meaning line 136 will never underflow/revert.

Consider removing the misleading comment at line 136.

3. Typo mistakes

It is not recommended to spell words incorrectly as this will decreases readability. However, this is issue is present in many contracts. Consider fixing the following typos to increase readability:

File: StRSR.sol Line 25, Line 541

    /// @audit zereod -> zeroed
25: *   If this happens, users balances are zereod out and StRSR is re-issued at a 1:1 exchange rate

      /// @audit appeneded -> appended
541:  //   r'.queue is r.queue with a new entry appeneded for (totalDrafts' - totalDraft) drafts

File: Fixed.sol Line 288

            /// @audit "Multiply-in" -> "Multiply in"
288:        // Multiply-in FIX_SCALE before dividing by y to preserve precision.

4. chainlinkFeed.latestRoundData() could give invalid price

In OracleLib.sol, the function price() does not check for a negative value being given from latestRoundData().

Consider verifying that int256 p is a non-negative integer so that, if a negative integer were to be given, an underflow will not happen when casting int256 p into a uint256() (at line 30).

Here is the code affected:

File: OracleLib.sol Line 14 - Line 31

14:    function price(AggregatorV3Interface chainlinkFeed, uint48 timeout)
15:        internal
16:        view
17:        returns (uint192)
18:    {
19:        (uint80 roundId, int256 p, , uint256 updateTime, uint80 answeredInRound) = chainlinkFeed
20:            .latestRoundData();
21:
22:        if (updateTime == 0 || answeredInRound < roundId) {
23:            revert StalePrice();
24:        }
25:        // Downcast is safe: uint256(-) reverts on underflow; block.timestamp assumed < 2^48
26:        uint48 secondsSince = uint48(block.timestamp - updateTime);
27:        if (secondsSince > timeout) revert StalePrice();
28:
29:        // {UoA/tok}
30:        return shiftl_toFix(uint256(p), -int8(chainlinkFeed.decimals()));
31:    }

5. Use named imports rather than unnamed imports

Using named imports (import {x} from "y.sol") will speed up compilation, and avoids polluting the namespace making flattened files smaller.

For example:

File: BackingManager.sol Line 4-12

4: import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
6: import "../interfaces/IAsset.sol";
7: import "../interfaces/IBackingManager.sol";
8: import "../interfaces/IMain.sol";
9: import "../libraries/Array.sol";
10: import "../libraries/Fixed.sol";
11: import "./mixins/Trading.sol";
12: import "./mixins/RecollateralizationLib.sol";

6. Avoid short variable names

Very short variable names like p, x, _a can make code less readable and potentially less maintainable.

For example, the following variable name is not very descriptive:

File: NonFiatCollateral.sol Line 46

        uint192 p = pricePerTarget.mul(pegPrice).mul(refPerTok());

7. require() should be used over assert()

Using assert() instead of require() prevents the use of error strings and causes a Panic error on failure. However, many contracts across the codebase are found to be doing this.

assert() creates a Panic(uint256) error instead of Error(string) created by require(). Nevertheless, Solidity’s documentation says:

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.

whereas

The require function either creates an error without any data or an error of type Error(string). It should be used to ensure valid conditions that cannot be detected until execution time. This includes conditions on inputs or return values from calls to external contracts.

Also, you can optionally provide a message string for require, but not for assert.

Source: docs.soliditylang.org/en/v0.8.17/control-structures.html#panic-via-assert-and-error-via-require

Consider using require() with informative error strings instead of assert().

here are some examples:

• File: TradeLib.sol Line 44
• File: TradeLib.sol Line 108-113
• File: TradeLib.sol Line 168
• File: RecollateralizationLib.sol Line 110

1. Use assembly to check for zero address

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

For example:

File: Auth.sol Line 94

        require(account != address(0), "cannot grant role to address 0");

The above zero address check can be written in assembly to save gas:

        assembly {
            if iszero(address(account)) {
                mstore(0x00, "cannot grant role to address 0")
                revert(0x00, 0x20)
            }

Other instances of this issue:

• File: ComponentRegistry.sol Line 37
• File: ComponentRegistry.sol Line 45
• File: ComponentRegistry.sol Line 53
• File: ComponentRegistry.sol Line 61
• File: ComponentRegistry.sol Line 69
• File: ComponentRegistry.sol Line 77
• File: ComponentRegistry.sol Line 85
• File: ComponentRegistry.sol Line 93
• File: ComponentRegistry.sol Line 101
• File: ComponentRegistry.sol Line 109

2. Functions that can only be called by users with correct permissions/roles can be marked payable

Functions with access control will cost less gas when marked as payable (assuming the caller has correct permissions). This is because the compiler doesn't need to check for msg.value, which saves approximately 20 gas per call.

Here are some instances of this issue:

• File: Auth.sol Line 148
• File: Auth.sol Line 157
• File: Auth.sol Line 165

3. Parameter uint48 val should be made full use of

In the following code, at line 816 (second require statement), instead of reading from unstakingDelay (state variable), you could read from the parameter uint48 val (local). This will save 1 SLOAD.

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L812-L817

812:    function setUnstakingDelay(uint48 val) public governance {
813:        require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
814:        emit UnstakingDelaySet(unstakingDelay, val);
815:        unstakingDelay = val;
816:        require(rewardPeriod * 2 <= unstakingDelay, "unstakingDelay/rewardPeriod incompatible");
817:    }

For example, the above may be refactored to:

812:    function setUnstakingDelay(uint48 val) public governance {
813:        require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");
814:        emit UnstakingDelaySet(unstakingDelay, val);
815:        require(rewardPeriod * 2 <= val, "unstakingDelay/rewardPeriod incompatible");
816:        unstakingDelay = val;
817:    }

Note: the require statement is put before unstakingDelay = val; to decrease gas wastage if a revert were to happen.

Here is another instance of this issue:

File: StRSR.sol Line 820-825

    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");
    }

In the above function, at the second require statement, instead of reading from rewardPeriod (state variable), you could read from the parameter uint48 val (local). This will save 1 SLOAD.

4. Unchecked math saves gas

Use the unchecked keyword to avoid unnecessary arithmetic checks and save gas when an underflow/overflow will not happen.

File: BasketHandler.sol #L70, #L78, #L218, #L227, #L262, #L286, #L337, #L397, #L416, #L437, #L530, #L548, #L586, #L643, #L707, #L725

70:        for (uint256 i = 0; i < length; ++i) self.refAmts[self.erc20s[i]] = FIX_ZERO;

78:        for (uint256 i = 0; i < length; ++i) {

218:        for (uint256 i = 0; i < config.erc20s.length; ++i) {

227:        for (uint256 i = 0; i < erc20s.length; ++i) {

262:        for (uint256 i = 0; i < erc20s.length; ++i) {

286:        for (uint256 i = 0; i < size; ++i) {

337:        for (uint256 i = 0; i < len; ++i) {

397:        for (uint256 i = 0; i < len; ++i) {

416:        for (uint256 i = 0; i < length; ++i) {

437:        for (uint256 i = 0; i < length; ++i) {

530:        for (uint256 i = 0; i < basketLength; ++i) {

548:        for (uint256 i = 0; i < basketLength; ++i) {

586:        for (uint256 i = 0; i < targetsLength; ++i) {

643:        for (uint256 i = 0; i < basketLength; ++i) {

707:        for (uint256 i = 0; i < erc20s.length; ++i) {

725:        for (uint256 i = 0; i < erc20s.length; ++i) {

5. Splitting require() statements that use && saves 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 &&.

Here are some examples of this issue:

• File: Auth.sol Line 181
• File: Auth.sol Line 188
• File: Broker.sol Line 135

6. Non-strict equalities (<=, >=) is less gas efficient than strict equalities (<, >)

Strict equalities will save gas (less opcodes)

For example:

File: Fixed.sol Line 101

    if (shiftLeft <= -96) return (rounding == CEIL ? 1 : 0); // 0 < uint.max / 10**77 < 0.5

In the code above, <= is used when < could be used instead:

    if (shiftLeft < -97) return (rounding == CEIL ? 1 : 0); // 0 < uint.max / 10**77 < 0.5

7. Using storage instead of memory for structs/arrays saves gas

When retrieving data from a storage location, assigning the data to a memory variable will cause all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incurring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct

For example:

File: BackingManager.sol Line 219-220

8. Revert as early as possible

Functions should revert as early as possible to prevent unnecessary gas wastage.

File: StRSR.sol Line 257-260

257:    function unstake(uint256 stakeAmount) external notPausedOrFrozen {
258:        address account = _msgSender();
259:        require(stakeAmount > 0, "Cannot withdraw zero");
260:        require(stakes[era][account] >= stakeAmount, "Not enough balance");

In the code above, address account is unnecessarily declared before require(stakeAmount > 0).

Consider swapping line 259 and line 258:

257:    function unstake(uint256 stakeAmount) external notPausedOrFrozen {
258:        require(stakeAmount > 0, "Cannot withdraw zero");
259:        address account = _msgSender();
260:        require(stakes[era][account] >= stakeAmount, "Not enough balance");