Reserve contest - Sathish9098 &#x27;s results

  import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
  import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
  import "../interfaces/IAssetRegistry.sol";
  import "../interfaces/IMain.sol";
  import "./mixins/Component.sol";

  import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
  import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
  import "../interfaces/IAsset.sol";
   import "../interfaces/IBackingManager.sol";
   import "../interfaces/IMain.sol";
  import "../libraries/Array.sol";
  import "../libraries/Fixed.sol";
  import "./mixins/Trading.sol";
  import "./mixins/RecollateralizationLib.sol";

  import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
  import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
  import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
  import "../interfaces/IAssetRegistry.sol";
  import "../interfaces/IBasketHandler.sol";
  import "../interfaces/IMain.sol";
  import "../libraries/Array.sol";
   import "./mixins/Component.sol";

  import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
  import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
  import "@openzeppelin/contracts/proxy/Clones.sol";
  import "../interfaces/IBroker.sol";
  import "../interfaces/IMain.sol";
  import "../interfaces/ITrade.sol";
  import "../libraries/Fixed.sol";
  import "./mixins/Component.sol";
  import "../plugins/trading/GnosisTrade.sol";

  import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
  import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
  import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
  import "../interfaces/IDistributor.sol";
  import "../interfaces/IMain.sol";
  import "../libraries/Fixed.sol";
  import "./mixins/Component.sol";

import "@openzeppelin/contracts-upgradeable/interfaces/IERC1271Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/cryptography/SignatureCheckerUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/cryptography/draft-EIP712Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

import "../interfaces/IStRSR.sol";
import "../interfaces/IMain.sol";
import "../libraries/Fixed.sol";
import "../libraries/Permit.sol";
import "./mixins/Component.sol";

[L-2] AVOID HARDCODED VALUES

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code

    165 :   require(share.rsrDist <= 10000, "RSR distribution too high");

    166:    require(share.rTokenDist <= 10000, "RToken distribution too high");

[L-3] LACK OF CHECKS THE INTEGER RANGES

The following methods lack checks on the following integer arguments, you can see the recommendations above.

Affected Source Code issuanceRate_,scalingRedemptionRate_,redemptionRateFloor_ is not checked to be != 0 during the init, nevertheless it’s checked in setIssuanceRate, setScalingRedemptionRate, setRedemptionRateFloor

FILE : StaticATokenLM.sol

   function init(
    IMain main_,
    string calldata name_,
    string calldata symbol_,
    string calldata mandate_,
    uint192 issuanceRate_,  //@audit  LACK OF CHECKS THE INTEGER RANGES
    uint192 scalingRedemptionRate_,    //@audit  LACK OF CHECKS THE INTEGER RANGES
    uint256 redemptionRateFloor_      //@audit  LACK OF CHECKS THE INTEGER RANGES
    ) external initializer {
    require(bytes(name_).length > 0, "name empty");
    require(bytes(symbol_).length > 0, "symbol empty");
    require(bytes(mandate_).length > 0, "mandate empty");
    __Component_init(main_);
    __ERC20_init(name_, symbol_);
    __ERC20Permit_init(name_);

    assetRegistry = main_.assetRegistry();
    basketHandler = main_.basketHandler();
    backingManager = main_.backingManager();
    furnace = main_.furnace();

    mandate = mandate_;
    setIssuanceRate(issuanceRate_);
    setScalingRedemptionRate(scalingRedemptionRate_);
    setRedemptionRateFloor(redemptionRateFloor_);
    }

Recommended Mitigation :

Need to make sure integer values not zero and not exceeding with its minimum or maximum values .

[L-4] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead

The abi.encodePacked function was meant to be simpler and more compact for encoding data. The abi.encode function can be useful when it comes to preventing collisions in hash functions.

(AAA, BBB) -> AAABBB
(AA, ABBB) -> AAABBB

function collisionExample(string memory _string1, string memory _string2) public pure returns (bytes32) { return keccak256(abi.encodePacked(_string1, _string2)); }

Let’s use a simple example (Example 1):

_string1 = AAA

_string2 = BBB

The result when we concatenate the strings and apply the hash function is:

0xf6568e65381c5fc6a447ddf0dcb848248282b798b2121d9944a87599b7921358 Now let us change the values to the following (Example 2):

_string1 = AA

_string2 = ABBB

Here is the result:

0xf6568e65381c5fc6a447ddf0dcb848248282b798b2121d9944a87599b7921358

Now let us use the same function, but this time with abi.encode.

function collisionExample2(string memory _string1, string memory _string2) public pure returns (bytes32) { return keccak256(abi.encode(_string1, _string2)); } With the first example (Example 1) the result is:

0xd6da8b03238087e6936e7b951b58424ff2783accb08af423b2b9a71cb02bd18b This is much different than using abi.encodePacked. Now the moment of truth (Example 2), will it result in the same hash?

0x54bc5894818c61a0ab427d51905bc936ae11a8111a8b373e53c8a34720957abb

  (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/aave/StaticATokenLM.sol#L146-L154)

  (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/aave/StaticATokenLM.sol#L174-L191)

  (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/aave/StaticATokenLM.sol#L214-L231)

NON-CRITICAL FINDINGS

[NC-1] USE A MORE RECENT VERSION OF SOLIDITY

Instances (25)

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)

2:  pragma solidity 0.8.9;

2: pragma solidity 0.8.9;

2:  pragma solidity 0.8.9;

   2 :  pragma solidity 0.8.9;

  2 : pragma solidity 0.8.9;

(https://docs.soliditylang.org/en/v0.8.15/natspec-format.html)

2 :  pragma solidity 0.8.9;

[NC- 2] NATSPEC COMMENTS SHOULD BE ADDED IN CONTRACTS

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation. In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

Recommendation NatSpec comments should be added in Contracts

[NC-3] TYPOS

Instances (13)

//@audit unregistering => meaning less word

159 : /// Register an asset, unregistering any previous asset with the same ERC20.

// @audit Mointain = > Maintain

89 :  /// Mointain the overall backing policy; handout assets otherwise

// @audit undercollateralized= > under collateralized

130 : * If we run out of capital and are still undercollateralized, we compromise

// @audit collateralized= > meaning less word

157 : * - Fully collateralized. All collateral meet balance requirements.

// @audit collateralized= > meaning less word

210 : // - We're fully collateralized

// @audit collateralized= > meaning less word

   272 :     /// ie, whether the protocol is currently fully collateralized

// @audit interepreted = > interpreted

  537 :      // As such, they're each interepreted as a map from target name -> target weight

FILE : Furnace.sol

    // @audit permisionlessly= > meaning less word used 

 10 :    * @notice A helper to melt RTokens slowly and permisionlessly.

      // @audit adjaacent= > adjacent

  109 :  // issuances, and so any particular issuance is actually the _difference_ between two adjaacent

   // @audit IssueQueue ,striaght ,TotalIssue 

  112:       // The way to keep an IssueQueue striaght in your head is to think of each TotalIssue item as a

  // @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

// @audit Overriden => Overridden

  570 :   /// Overriden in StRSRVotes to handle rebases

[NC-4] FOR FUNCTIONS, FOLLOW SOLIDITY STANDARD NAMING CONVENTIONS

Description

The above codes don’t follow Solidity’s standard naming convention,

internal and private functions : the mixedCase format starting with an underscore (_mixedCase starting with an underscore)

public and external functions : only mixedCase

constant variable : UPPERCASEWITH_UNDERSCORES (separated by uppercase and underscore)

  75 : function setFrom(Basket storage self, Basket storage other) internal {

87 : function add(
    Basket storage self,
    IERC20 tok,
    uint192 weight
   ) internal {


 650 :   function requireValidCollArray(IERC20[] calldata erc20s) internal view {

[NC-5] EXPRESSIONS FOR CONSTANT VALUES SUCH AS A CALL TO KECCAK256(), SHOULD USE IMMUTABLE RATHER THAN CONSTANT

While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

Instances (3)

   31:  address public constant FURNACE = address(1);

   32:  address public constant ST_RSR = address(2);

     bytes32 private constant _PERMIT_TYPEHASH =
    keccak256(
        "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"
    );

#0 - c4-judge
2023-01-25T02:42:10Z

0xean marked the issue as grade-b

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0xA5DF, 0xSmartContract, 0xhacksmithh, AkshaySrivastav, Awesome, Aymen0909, Bauer, Bnke0x0, Breeje, Budaghyan, Cyfrin, Madalad, NoamYakov, RHaO-sec, Rageur, RaymondFam, ReyAdmirado, Rolezn, SAAJ, SaharDevep, Sathish9098, __141345__, amshirif, arialblack14, c3phas, carlitox477, chaduke, delfin454000, descharre, nadin, oyc_109, pavankv, saneryee, shark

Awards

72.4433 USDC - $72.44

Labels

bug

G (Gas Optimization)

grade-b

edited-by-warden

G-31

External Links

Link to GitHub issue

GAS REPORTS

[GAS-1] ++I/I++ OR --I/I-- SHOULD BE UNCHECKED{++I}/UNCHECKED{I++} OR UNCHECKED{--I}/UNCHECKED{I--}WHEN IT IS NOT POSSIBLE FOR THEM TO OVERFLOW, AS IS THE CASE WHEN USED IN FOR- AND WHILE-LOOPS

Instances (49) :

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

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

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

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

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

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

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

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

  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) {

553 :   for (targetIndex = 0; targetIndex < targetsLength; ++targetIndex) {

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

597 :   for (uint256 j = 0; j < backupLength && size < backup.max; ++j) {

611 :    for (uint256 j = 0; j < backupLength && assigned < size; ++j) {

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

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

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

 108 :  for (uint256 i = 0; i < destinations.length(); ++i) {

 133 :  for (uint256 i = 0; i < numTransfers; i++) {

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

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

 303:    for (uint256 i = 0; i < basketSize; ++i) {

 329:   for (uint256 i = 0; i < basketSize; ++i) {

334:    for (uint256 i = 0; i < basketSize; ++i) {

478 :   for (uint256 i = 0; i < erc20length; ++i) {

501:     for (uint256 i = 0; i < erc20length; ++i) {

674 :   for (uint256 i = 0; i < tokensLen; ++i) {

683:      for (uint256 i = 0; i < tokensLen; ++i) {

711:    for (uint256 i = 0; i < queue.tokens.length; ++i) {

757:    for (uint256 i = 0; i < tokensLen; ++i) {

793:    for (uint256 i = 0; i < tokensLen; ++i) {

[GAS-2] NOT USING THE NAMED RETURN VARIABLES WHEN A FUNCTION RETURNS, WASTES DEPLOYMENT GAS

Instances (3) :

   162 :  function _registerIgnoringCollisions(IAsset asset) private returns (bool swapped) {

   325 :   function lotPrice() external view returns (uint192 lotLow, uint192 lotHigh) {

    634:  function queueBounds(address account) external view returns (uint256 left, uint256 right) {

[GAS-3] ADD UNCHECKED {} FOR SUBTRACTIONS WHERE THE OPERANDS CANNOT UNDERFLOW BECAUSE OF A PREVIOUS REQUIRE() OR IF-STATEMENT . This saves 30-40 gas

Instances (1) :

SOLUTION:

require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }

     141 :  if (req.sellAmount > bal) stRSR.seizeRSR(req.sellAmount - bal);

[GAS-4] CAN MAKE THE VARIABLE OUTSIDE THE LOOP TO SAVE GAS.ITS POSSIBLE TO SAVE 10 GAS

Instances (13) :

    for (uint256 i = 0; i < length; ++i) {
        IAsset asset = assetRegistry.toAsset(erc20s[i]);  //@audit variable outside the loop

        uint192 req = needed.mul(basketHandler.quantity(erc20s[i]), CEIL);  //@audit variable outside the loop
        if (asset.bal(address(this)).gt(req)) {
            // delta: {qTok}, the excess quantity of this asset that we hold
            uint256 delta = asset.bal(address(this)).minus(req).shiftl_toUint(     //@audit variable outside the loop
                int8(IERC20Metadata(address(erc20s[i])).decimals())                     //@audit variable outside the loop
            );
            // no div-by-0: Distributor guarantees (totals.rTokenTotal + totals.rsrTotal) > 0
            // initial division is intentional here! We'd rather save the dust than be unfair
            toRSR[i] = (delta / (totals.rTokenTotal + totals.rsrTotal)) * totals.rsrTotal;
            toRToken[i] = (delta / (totals.rTokenTotal + totals.rsrTotal)) * totals.rTokenTotal;
        }
    }


    for (uint256 i = 0; i < length; ++i) {
        IERC20Upgradeable erc20 = IERC20Upgradeable(address(erc20s[i]));  //@audit variable outside the loop
        if (toRToken[i] > 0) erc20.safeTransfer(address(rTokenTrader), toRToken[i]);
        if (toRSR[i] > 0) erc20.safeTransfer(address(rsrTrader), toRSR[i]);
    }

    for (uint256 i = 0; i < size; ++i) {
        CollateralStatus s = assetRegistry.toColl(basket.erc20s[i]).status();  //@audit variable outside the loop
        if (s.worseThan(status_)) status_ = s;
    }


  for (uint256 i = 0; i < length; ++i) {
        ICollateral coll = assetRegistry.toColl(basket.erc20s[i]);   //@audit variable outside the loop
        if (coll.status() == CollateralStatus.DISABLED) return FIX_ZERO;

        uint192 refPerTok = coll.refPerTok();       //@audit variable outside the loop
        // If refPerTok is 0, then we have zero of coll's reference unit.
        // We know that basket.refAmts[basket.erc20s[i]] > 0, so we have no baskets.
        if (refPerTok == 0) return FIX_ZERO;

        // {tok}
        uint192 bal = coll.bal(account);        //@audit variable outside the loop

        // {tok/BU} = {ref/BU} / {ref/tok}.  0-division averted by condition above.
        uint192 q = basket.refAmts[basket.erc20s[i]].div(refPerTok, CEIL);        //@audit variable outside the loop

        // {BU} = {tok} / {tok/BU}.  q > 0 because q = (n).div(_, CEIL) and n > 0
        baskets = fixMin(baskets, bal.div(q));
    }
}

   for (uint256 i = 0; i < basketLength; ++i) {
        IERC20 erc20 = config.erc20s[i];                  //@audit variable outside the loop

(https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Distributor.sol#L108-L128)

  for (uint256 i = 0; i < numTransfers; i++) {
        Transfer memory t = transfers[i];     //@audit variable outside the loop
        IERC20Upgradeable(address(t.erc20)).safeTransferFrom(from, t.addrTo, t.amount);
    }

    for (uint256 i = 0; i < length; ++i) {
        RevenueShare storage share = distribution[destinations.at(i)];   //@audit variable outside the loop
        revTotals.rTokenTotal += share.rTokenDist;   
        revTotals.rsrTotal += share.rsrDist;  
    }

[GAS-5] USE REQUIRE INSTEAD OF ASSERT

Instances (2) :

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

     249 :  assert(tradesOpen == 0 && !basketHandler.fullyCollateralized());

     556 :  assert(targetIndex < targetsLength);

[GAS-6] USING STORAGE INSTEAD OF MEMORY FOR STRUCTS/ARRAYS SAVES GAS

When fetching data from a storage location, assigning the data to a memory variable causes 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 declearing 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 incuring 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

Instances (3) :

    169 :  uint192[] memory refAmts = new uint192[](basket.erc20s.length);

    541 :   uint192[] memory goodWeights = new uint192[](targetsLength);

    545 :   uint192[] memory totalWeights = new uint192[](targetsLength);

[ GAS-7] <X> += <Y> COSTS MORE GAS THAN <X> = <X> + <Y> FOR STATE VARIABLES . FOR EVERY CALL CAN SAVE 13 GAS

Instances (9) :

    636:  nonce += 1;

FILE : Furnace.sol

   81 :   lastPayout += numPeriods * period;

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

  687:    liabilities[IERC20(queue.tokens[i])] -= amt[i];

 761:    liabilities[IERC20(queue.tokens[i])] -= amtDeposits[i];

 229 :    stakeRSR += rsrAmount;

 516 :   payoutLastPaid += numPeriods * rewardPeriod;

 549 :   draftRSR += rsrAmount;

 721:    totalStakes -= amount;

[GAS-8] State variable nonce should be cached with stack variable to save gas

Instances (1) :

   636 :    nonce += 1;

   646:    emit BasketSet(nonce, basket.erc20s, refAmts, disabled);

[GAS-9] Instead of using operator && on single require . Using double REQUIRE checks can save more gas. After splitting require statement its possible save 8 gas

Instances (10) :

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

FILE : Deployer.sol

        109 :  require(owner != address(0) && owner != address(this), "invalid owner");

FILE : Furnace.sol

      89 :  require(period_ > 0 && period_ <= MAX_PERIOD, "invalid period");

    410 :  require(queue.left <= endId && endId <= queue.right, "out of range");


   590 :   require(val > 0 && val <= MAX_ISSUANCE_RATE, "invalid issuanceRate");

  623:    require(index >= item.left && index < item.right, "out of range");

  741:   require(queue.left <= endId && endId <= queue.right, "out of range");

  813:   require(uint192(low) >= 1e9 && uint192(high) <= 1e27, "BU rate out of range");

 813:     require(val > 0 && val <= MAX_UNSTAKING_DELAY, "invalid unstakingDelay");

 821:     require(val > 0 && val <= MAX_REWARD_PERIOD, "invalid rewardPeriod");

[GAS-10] Use assembly to check for address(0)

Instances (5) :

             57 : require(address(gnosis_) != address(0), "invalid Gnosis address");

             58 :  require(
             address(tradeImplementation_) != address(0),
             "invalid Trade Implementation address"
            );

FILE : Deployer.sol

       (https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Deployer.sol#L48-L65)

       109 :  require(owner != address(0) && owner != address(this), "invalid owner");

       162 :   require(dest != address(0), "dest cannot be zero");

[GAS-11] Use calldata instead of memory for function arguments that do not get mutated

Instances (4) :

FILE : Deployer.sol

    103 :  string memory name,
    
     104 :  string memory symbol,