Gas Optimizations - Githubissues

Table of Contents:

Copying a full array from storage to memory isn't optimal
Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it
Caching storage values in memory
Unchecking arithmetics operations that can't underflow/overflow
Boolean comparisons
> 0 is less efficient than != 0 for unsigned integers (with proof)
Splitting require() statements that use && saves gas
Usage of assert() instead of require()
Amounts should be checked for 0 before calling a transfer
An array's length should be cached to save gas in for-loops
++i costs less gas compared to i++ or i += 1
Do not pre-declare variable with default values
Increments can be unchecked
Public functions to external
No need to explicitly initialize variables with default values
Upgrade pragma to at least 0.8.4
PriceOracleImplementation.cEtherAddress variable should be immutable
Reduce the size of error messages (Long revert Strings)
Use Custom Errors instead of Revert Strings to save Gas

Copying a full array from storage to memory isn't optimal

Here, what's happening is a full copy of a storage array in memory, and then a second copy of each memory element in a CToken struct:

File: Comptroller.sol
590:         CToken[] memory assets = accountAssets[account]; //@audit this here is a VERY expensive copy of a whole storage array in memory (as many SLOADs as there are elements)
591:         for (uint i = 0; i < assets.length; i++) {
592:             CToken asset = assets[i]; //@audit here is a copy of a memory value in a memory variable

The code should be optimized that way:

        CToken[] storage assets = accountAssets[account]; //@audit using storage keyword vs memory

This way, the amount of MSTOREs gets divided by 2 and no MLOADs are then necessary

Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it

To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.

The effect can be quite significant.

As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.

Instances include (check the @audit tags):

contracts/Comptroller.sol:
   268:         if (!markets[cToken].isListed) { //@audit gas: Should declare "Market storage _ctoken = markets[cToken]" and use it
   273:         if (cToken != address(nftMarket) && !markets[cToken].accountMembership[redeemer]) { //@audit gas: Should use suggested storage var _ctoken
   318:         if (!markets[cToken].isListed) { //@audit gas: Should declare "Market storage _ctoken = markets[cToken]" and use it
   322:         if (!markets[cToken].accountMembership[borrower]) { //@audit gas: Should use suggested storage var _ctoken
   333:             assert(markets[cToken].accountMembership[borrower]); //@audit gas: Should use suggested storage var _ctoken
  1067:             if (compSupplyState[address(cToken)].index == 0 && compSupplyState[address(cToken)].block == 0) { //@audit gas: should declare CompMarketState storage _compMarketState = compSupplyState[address(cToken)]; and use it
  1068:                 compSupplyState[address(cToken)] = CompMarketState({  //@audit gas: should write with suggested _compMarketState.index = compInitialIndex and _compMarketState.block = safe32(getBlockNumber(), "block number exceeds 32 bits")
  1074:             if (compBorrowState[address(cToken)].index == 0 && compBorrowState[address(cToken)].block == 0) {//@audit gas: should use suggested _compMarketState
  1075:                 compBorrowState[address(cToken)] = CompMarketState({//@audit gas: should use suggested _compMarketState
  1345:         if (markets[address(cNft)].isListed) { //@audit should declare Market storage _market = markets[address(cNft)]; and use it
  1355:         markets[address(cNft)] = Market({isListed: true, isComped: false, collateralFactorMantissa: _collateralFactorMantissa}); // @audit should use suggested _market

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

contracts/CErc20.sol:
   37:         EIP20Interface(underlying).totalSupply(); //@audit gas: should use underlying_ to avoid an SLOAD
  172:         EIP20NonStandardInterface token = EIP20NonStandardInterface(underlying); //@audit gas: should cache underlying
  173:         uint balanceBefore = EIP20Interface(underlying).balanceOf(address(this)); //@audit gas: should use cached underlying
  193:         uint balanceAfter = EIP20Interface(underlying).balanceOf(address(this)); //@audit gas: should use cached underlying

contracts/CNft.sol:
   64:                     (bool checkSuccess, bytes memory result) = underlying.staticcall(punkIndexToAddress); //@audit this is a SLOAD in a loop. Should cache underlying before if/else statement and use cached value here
   68:                     (bool buyPunkSuccess, ) = underlying.call(data); //@audit this is a SLOAD in a loop. Should cache underlying before if/else statement and use cached value here
   73:                     IERC721(underlying).safeTransferFrom(msg.sender, address(this), tokenIds[i], ""); //@audit this is a SLOAD in a loop. Should cache underlying before if/else statement and use cached value here
  147:                     (bool transferPunkSuccess, ) = underlying.call(data); //@audit this is a SLOAD in a loop. Should cache underlying before if/else statement and use cached value here
  152:                     IERC721(underlying).safeTransferFrom(address(this), msg.sender, tokenIds[i], ""); //@audit this is a SLOAD in a loop. Should cache underlying before if/else statement and use cached value here

contracts/Comptroller.sol:
   328:             if (err != Error.NO_ERROR) { //@audit gas: Error.NO_ERROR SLOAD 1
   350:         if (err != Error.NO_ERROR) {  //@audit gas: Error.NO_ERROR SLOAD 2
   362:         return uint(Error.NO_ERROR); //@audit gas: Error.NO_ERROR SLOAD 3
   603:             vars.oraclePriceMantissa = oracle.getUnderlyingPrice(asset); //@audit gas: oracle SLOAD in for-loop. Cache it before the loop
   630:         uint256 nftBalance = nftMarket.totalBalance(account);//@audit gas: nftMarket SLOAD 1
   633:             vars.nftOraclePriceMantissa =  nftOracle.getUnderlyingPrice(nftMarket); //@audit gas: nftMarket SLOAD 2
   638:             vars.nftCollateralFactor = Exp({mantissa: markets[address(nftMarket)].collateralFactorMantissa});  //@audit gas: markets[address(nftMarket)].collateralFactorMantissa SLOAD 1, nftMarket SLOAD 3
   641:             vars.sumCollateral = mul_ScalarTruncateAddUInt(mul_(vars.nftOraclePrice, markets[address(nftMarket)].collateralFactorMantissa), nftBalance, vars.sumCollateral);  //@audit gas: markets[address(nftMarket)].collateralFactorMantissa SLOAD 2, nftMarket SLOAD 4
   642:             if (cAssetModify == address(nftMarket)) {  //@audit gas: nftMarket SLOAD 5
   645:                 vars.sumBorrowPlusEffects = mul_ScalarTruncateAddUInt(mul_(vars.nftOraclePrice, markets[address(nftMarket)].collateralFactorMantissa), redeemTokens, vars.sumBorrowPlusEffects);  //@audit gas: markets[address(nftMarket)].collateralFactorMantissa SLOAD 3, nftMarket SLOAD 6
   667:         uint priceBorrowedMantissa = oracle.getUnderlyingPrice(CToken(cTokenBorrowed)); //@audit gas: oracle SLOAD 1
   668:         uint priceCollateralMantissa = oracle.getUnderlyingPrice(CToken(cTokenCollateral)); //@audit gas: oracle SLOAD 2
   702:         require(cNftCollateral == address(nftMarket), "cNFT is from the wrong comptroller");  //@audit gas: nftMarket SLOAD 1
   706:         uint priceCollateralMantissa = nftOracle.getUnderlyingPrice(nftMarket); //@audit gas: nftMarket SLOAD 2
   788:         emit NewCloseFactor(oldCloseFactorMantissa, closeFactorMantissa); //@audit gas: SLOAD, should emit newCloseFactorMantissa instead of closeFactorMantissa

contracts/CToken.sol:
   976:         if (repayBorrowError != uint(Error.NO_ERROR)) { //@audit gas: uint(Error.NO_ERROR) SLOAD 1
   986:         require(amountSeizeError == uint(Error.NO_ERROR), "LIQUIDATE_COMPTROLLER_CALCULATE_AMOUNT_SEIZE_FAILED");//@audit gas: uint(Error.NO_ERROR) SLOAD 2
  1000:         require(seizeError == uint(Error.NO_ERROR), "token seizure failed"); //@audit gas: uint(Error.NO_ERROR) SLOAD 3
  1005:         return (uint(Error.NO_ERROR), actualRepayAmount);//@audit gas: uint(Error.NO_ERROR) SLOAD 4
  1073:         if (repayBorrowError != uint(Error.NO_ERROR)) {//@audit gas: uint(Error.NO_ERROR) SLOAD 1
  1083:         require(amountSeizeError == uint(Error.NO_ERROR), "LIQUIDATE_COMPTROLLER_CALCULATE_AMOUNT_SEIZE_FAILED");//@audit gas: uint(Error.NO_ERROR) SLOAD 2
  1101:         return (uint(Error.NO_ERROR), actualRepayAmount);//@audit gas: uint(Error.NO_ERROR) SLOAD 3

contracts/Oracles/UniswapV2PriceOracle.sol:
   26:             numPairObservations[pair] > 0 && //@audit should cache numPairObservations[pair]
   27:             (block.timestamp - pairObservations[pair][(numPairObservations[pair] - 1) % OBSERVATION_BUFFER_SIZE].timestamp) <= MIN_TWAP_TIME //@audit should cache pairObservations[pair]
   32:         pairObservations[pair][numPairObservations[pair]++ % OBSERVATION_BUFFER_SIZE] = Observation( //@audit should use cached pairObservations[pair] and cached numPairObservations[pair]++. THEN it should SSTORE in [numPairObservations[pair] the cached value instead of doing [numPairObservations[pair]++
  130:         if (lastObservation.timestamp > block.timestamp - MIN_TWAP_TIME) {  //@audit should cache lastObservation.timestamp
  136:             block.timestamp - lastObservation.timestamp >= MIN_TWAP_TIME,  //@audit should use cached lastObservation.timestamp
  142:             return (px0Cumulative - lastObservation.price0Cumulative) / (block.timestamp - lastObservation.timestamp);  //@audit should use cached lastObservation.timestamp
  151:         if (lastObservation.timestamp > block.timestamp - MIN_TWAP_TIME) { //@audit should use cached lastObservation.timestamp
  157:             block.timestamp - lastObservation.timestamp >= MIN_TWAP_TIME, //@audit should use cached lastObservation.timestamp
  163:             return (px1Cumulative - lastObservation.price1Cumulative) / (block.timestamp - lastObservation.timestamp); //@audit should use cached lastObservation.timestamp

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tags for more details):

contracts/Oracles/UniswapV2PriceOracle.sol:
  129:         Observation storage lastObservation = pairObservations[pair][(length - 1) % OBSERVATION_BUFFER_SIZE]; //@audit gas: should be unchecked due to L128
  132:             lastObservation = pairObservations[pair][(length - 2) % OBSERVATION_BUFFER_SIZE];//@audit gas: should be unchecked due to L131
  150:         Observation storage lastObservation = pairObservations[pair][(length - 1) % OBSERVATION_BUFFER_SIZE];//@audit gas: should be unchecked due to L149
  153:             lastObservation = pairObservations[pair][(length - 2) % OBSERVATION_BUFFER_SIZE];//@audit gas: should be unchecked due to L152

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) here (same for require statements):

Comptroller.sol:142:        if (marketToJoin.accountMembership[borrower] == true) {
Comptroller.sol:997:        require(msg.sender == admin || state == true, "only admin can unpause");
Comptroller.sol:1011:        require(msg.sender == admin || state == true, "only admin can unpause");
Comptroller.sol:1020:        require(msg.sender == admin || state == true, "only admin can unpause");
Comptroller.sol:1029:        require(msg.sender == admin || state == true, "only admin can unpause");
Comptroller.sol:1065:            require(market.isListed == true, "comp market is not listed");
Comptroller.sol:1226:            if (borrowers == true) {
Comptroller.sol:1233:            if (suppliers == true) {
Comptroller.sol:1325:            borrowGuardianPaused[address(cToken)] == true &&

`> 0` is less efficient than `!= 0` for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

Oracles/CNftPriceOracle.sol:63:            cNfts.length > 0 && cNfts.length == nftxTokens.length,
Oracles/UniswapV2PriceOracle.sol:67:                    reserve0 > 0 && reserve1 > 0,
Oracles/UniswapV2PriceOracle.sol:91:                    reserve0 > 0,
Oracles/UniswapV2PriceOracle.sol:115:                    reserve1 > 0,
Oracles/UniswapV2PriceOracle.sol:128:        require(length > 0, "UniswapV2PriceOracle: No observations.");
Oracles/UniswapV2PriceOracle.sol:149:        require(length > 0, "UniswapV2PriceOracle: No observations.");
CToken.sol:37:        require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");

Also, please enable the Optimizer.

Splitting `require()` statements that use `&&` saves gas

If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement:

contracts/CNft.sol:
  66:                     require(checkSuccess && nftOwner == msg.sender, "Not the NFT owner");

contracts/Comptroller.sol:
  947:         require(numMarkets != 0 && numMarkets == numBorrowCaps, "invalid input");

contracts/CToken.sol:
  33:         require(accrualBlockNumber == 0 && borrowIndex == 0, "market may only be initialized once");

contracts/Oracles/CNftPriceOracle.sol:
  63:             cNfts.length > 0 && cNfts.length == nftxTokens.length,

contracts/Oracles/UniswapV2PriceOracle.sol:
  67:                     reserve0 > 0 && reserve1 > 0,

Usage of assert() instead of require()

Between solc 0.4.10 and 0.8.0, require() used REVERT (0xfd) opcode which refunded remaining gas on failure while assert() used INVALID (0xfe) opcode which consumed all the supplied gas. (see https://docs.soliditylang.org/en/v0.8.1/control-structures.html#error-handling-assert-require-revert-and-exceptions).
require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking (properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix). From the current usage of assert, my guess is that they can be replaced with require, unless a Panic really is intended.

Here are the assert locations:

Comptroller.sol:207:        assert(assetIndex < len);
Comptroller.sol:333:            assert(markets[cToken].accountMembership[borrower]);

Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

I suggest adding a non-zero-value check here:

File: CErc20.sol
135:     function sweepToken(EIP20NonStandardInterface token) external {
136:      require(address(token) != underlying, "CErc20::sweepToken: can not sweep underlying token");
137:      uint256 balance = token.balanceOf(address(this));
138:      token.transfer(admin, balance); //@audit gas: should check for balance == 0 before transfer
139:     }

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

Oracles/CNftPriceOracle.sol:66:        for (uint256 i = 0; i < cNfts.length; ++i) {
Oracles/UniswapV2PriceOracle.sol:42:        for (uint256 i = 0; i < pairs.length; ++i) {
CEther.sol:178:        for (i = 0; i < bytes(message).length; i++) {
CNft.sol:176:        for (uint256 i; i < vars.length; ++i) {
Comptroller.sol:591:        for (uint i = 0; i < assets.length; i++) {
Comptroller.sol:928:        for (uint i = 0; i < allMarkets.length; i ++) {
Comptroller.sol:1223:        for (uint i = 0; i < cTokens.length; i++) {
Comptroller.sol:1229:                for (uint j = 0; j < holders.length; j++) {
Comptroller.sol:1235:                for (uint j = 0; j < holders.length; j++) {
Comptroller.sol:1240:        for (uint j = 0; j < holders.length; j++) {
ERC1155Enumerable.sol:51:        for (uint256 i; i < ids.length; ++i) {

`++i` costs less gas compared to `i++` or `i += 1`

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

The same is also true for i--.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

CEther.sol:178:        for (i = 0; i < bytes(message).length; i++) {
Comptroller.sol:119:        for (uint i = 0; i < len; i++) {
Comptroller.sol:199:        for (uint i = 0; i < len; i++) {
Comptroller.sol:212:        storedList.length--; //@audit use --storedList.length
Comptroller.sol:591:        for (uint i = 0; i < assets.length; i++) {
Comptroller.sol:928:        for (uint i = 0; i < allMarkets.length; i ++) {
Comptroller.sol:949:        for(uint i = 0; i < numMarkets; i++) {
Comptroller.sol:1223:        for (uint i = 0; i < cTokens.length; i++) {
Comptroller.sol:1229:                for (uint j = 0; j < holders.length; j++) {
Comptroller.sol:1235:                for (uint j = 0; j < holders.length; j++) {
Comptroller.sol:1240:        for (uint j = 0; j < holders.length; j++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Do not pre-declare variable with default values

One of the practices in the project is to pre-declare variables before assigning a value to them. This is not necessary and actually costs some gas (MSTOREs and MLOADs).

As an example, consider going from:

File: Comptroller.sol
715:         uint seizeTokens;
716:         Exp memory numerator;
717:         Exp memory denominator;
718:         Exp memory ratio;
719: 
720:         numerator = mul_(Exp({mantissa: liquidationIncentiveMantissa}), Exp({mantissa: priceBorrowedMantissa}));
721:         denominator = Exp({mantissa: priceCollateralMantissa});
722:         ratio = div_(numerator, denominator);
723: 
724:         seizeTokens = truncate(mul_(ratio, Exp({mantissa: actualRepayAmount})));

to:

        Exp memory numerator = mul_(Exp({mantissa: liquidationIncentiveMantissa}), Exp({mantissa: priceBorrowedMantissa}));
        Exp memory denominator = Exp({mantissa: priceCollateralMantissa});
        Exp memory ratio = div_(numerator, denominator);

        uint seizeTokens = truncate(mul_(ratio, Exp({mantissa: actualRepayAmount})));

Same for the following code:

File: Comptroller.sol
680:         uint seizeTokens;
681:         Exp memory numerator;
682:         Exp memory denominator;
683:         Exp memory ratio;
684: 
685:         numerator = mul_(Exp({mantissa: liquidationIncentiveMantissa}), Exp({mantissa: priceBorrowedMantissa}));
686:         denominator = mul_(Exp({mantissa: priceCollateralMantissa}), Exp({mantissa: exchangeRateMantissa}));
687:         ratio = div_(numerator, denominator);
688: 
689:         seizeTokens = mul_ScalarTruncate(ratio, actualRepayAmount);

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

Oracles/CNftPriceOracle.sol:66:        for (uint256 i = 0; i < cNfts.length; ++i) {
Oracles/UniswapV2PriceOracle.sol:42:        for (uint256 i = 0; i < pairs.length; ++i) {
CNft.sol:50:        for (uint256 i; i < length; ++i) {
CNft.sol:62:                for (uint256 i; i < length; ++i) {
CNft.sol:72:                for (uint256 i; i < length; ++i) {
CNft.sol:98:        for (uint256 i; i < length; ++i) {
CNft.sol:122:        for (uint256 i; i < length; ++i) {
CNft.sol:145:                for (uint256 i; i < length; ++i) {
CNft.sol:151:                for (uint256 i; i < length; ++i) {
CNft.sol:176:        for (uint256 i; i < vars.length; ++i) {
ERC1155Enumerable.sol:51:        for (uint256 i; i < ids.length; ++i) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}

The risk of overflow is inexistant for uint256 here.

Public functions to external

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

_setInterestRateModel(InterestRateModel) should be declared external:
    - CToken._setInterestRateModel(InterestRateModel) (contracts/CToken.sol#1452-1460)

enterMarkets(address[]) should be declared external:
    - Comptroller.enterMarkets(address[]) (contracts/Comptroller.sol#115-126)
getAccountLiquidity(address) should be declared external:
    - Comptroller.getAccountLiquidity(address) (contracts/Comptroller.sol#533-537)
getHypotheticalAccountLiquidity(address,address,uint256,uint256) should be declared external:
    - Comptroller.getHypotheticalAccountLiquidity(address,address,uint256,uint256) (contracts/Comptroller.sol#559-566)
_setPriceOracle(PriceOracle) should be declared external:
    - Comptroller._setPriceOracle(PriceOracle) (contracts/Comptroller.sol#741-757)
_setNftPriceOracle(NftPriceOracle) should be declared external:
    - Comptroller._setNftPriceOracle(NftPriceOracle) (contracts/Comptroller.sol#764-774)
_setPauseGuardian(address) should be declared external:
    - Comptroller._setPauseGuardian(address) (contracts/Comptroller.sol#977-992)
_setMintPaused(address,bool) should be declared external:
    - Comptroller._setMintPaused(address,bool) (contracts/Comptroller.sol#994-1006)
_setBorrowPaused(CToken,bool) should be declared external:
    - Comptroller._setBorrowPaused(CToken,bool) (contracts/Comptroller.sol#1008-1016)
_setTransferPaused(bool) should be declared external:
    - Comptroller._setTransferPaused(bool) (contracts/Comptroller.sol#1018-1025)
_setSeizePaused(bool) should be declared external:
    - Comptroller._setSeizePaused(bool) (contracts/Comptroller.sol#1027-1034)
_become(Unitroller) should be declared external:
    - Comptroller._become(Unitroller) (contracts/Comptroller.sol#1036-1039)
claimComp(address) should be declared external:
    - Comptroller.claimComp(address) (contracts/Comptroller.sol#1200-1202)
_grantComp(address,uint256) should be declared external:
    - Comptroller._grantComp(address,uint256) (contracts/Comptroller.sol#1270-1275)
_setCompSpeed(CToken,uint256) should be declared external:
    - Comptroller._setCompSpeed(CToken,uint256) (contracts/Comptroller.sol#1282-1285)
_setContributorCompSpeed(address,uint256) should be declared external:
    - Comptroller._setContributorCompSpeed(address,uint256) (contracts/Comptroller.sol#1292-1306)
getAllMarkets() should be declared external:
    - Comptroller.getAllMarkets() (contracts/Comptroller.sol#1313-1315)

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

Oracles/CNftPriceOracle.sol:66:        for (uint256 i = 0; i < cNfts.length; ++i) {
Oracles/UniswapV2PriceOracle.sol:41:        uint256 numberUpdated = 0;
Oracles/UniswapV2PriceOracle.sol:42:        for (uint256 i = 0; i < pairs.length; ++i) {
CEther.sol:178:        for (i = 0; i < bytes(message).length; i++) { //@audit declared "uint i" just above, which already defaults to 0
CNft.sol:49:        uint256 totalAmount = 0;
CNft.sol:97:        uint256 totalAmount = 0;
CNft.sol:119:        uint256 totalAmount = 0;
Comptroller.sol:119:        for (uint i = 0; i < len; i++) {
Comptroller.sol:199:        for (uint i = 0; i < len; i++) {
Comptroller.sol:591:        for (uint i = 0; i < assets.length; i++) {
Comptroller.sol:928:        for (uint i = 0; i < allMarkets.length; i ++) {
Comptroller.sol:949:        for(uint i = 0; i < numMarkets; i++) {
Comptroller.sol:1223:        for (uint i = 0; i < cTokens.length; i++) {
Comptroller.sol:1229:                for (uint j = 0; j < holders.length; j++) {
Comptroller.sol:1235:                for (uint j = 0; j < holders.length; j++) {
Comptroller.sol:1240:        for (uint j = 0; j < holders.length; j++) {
CToken.sol:81:        uint startingAllowance = 0;

I suggest removing explicit initializations for default values.

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
Optimizer improvements in packed structs (>= 0.8.3)
Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Consider upgrading pragma to at least 0.8.4:

Oracles/CNftPriceOracle.sol:2:pragma solidity ^0.8.0;
Oracles/UniswapV2PriceOracle.sol:2:pragma solidity ^0.8.0;
CNft.sol:2:pragma solidity ^0.8.0;
ERC1155Enumerable.sol:2:pragma solidity ^0.8.0;

`PriceOracleImplementation.cEtherAddress` variable should be immutable

This variable is only set in the constructor and is never edited after that:

File: PriceOracleImplementation.sol
10:     address public cEtherAddress; //@audit gas: should be immutable as it's only set in the constructor

Consider marking it as immutable.

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

Oracles/CNftPriceOracle.sol:64:            "CNftPriceOracle: `cNfts` and `nftxTokens` must have nonzero, equal lengths."
Oracles/CNftPriceOracle.sol:70:                "CNftPriceOracle: Cannot overwrite existing address mappings."
Oracles/CNftPriceOracle.sol:90:            "CNftPriceOracle: No NFTX token for cNFT."
Oracles/UniswapV2PriceOracle.sol:68:                    "UniswapV2PriceOracle: Division by zero."
Oracles/UniswapV2PriceOracle.sol:92:                    "UniswapV2PriceOracle: Division by zero."
Oracles/UniswapV2PriceOracle.sol:116:                    "UniswapV2PriceOracle: Division by zero."
Oracles/UniswapV2PriceOracle.sol:128:        require(length > 0, "UniswapV2PriceOracle: No observations.");
Oracles/UniswapV2PriceOracle.sol:131:            require(length > 1, "UniswapV2PriceOracle: Only one observation.");
Oracles/UniswapV2PriceOracle.sol:137:            "UniswapV2PriceOracle: Bad TWAP time."
Oracles/UniswapV2PriceOracle.sol:149:        require(length > 0, "UniswapV2PriceOracle: No observations.");
Oracles/UniswapV2PriceOracle.sol:152:            require(length > 1, "UniswapV2PriceOracle: Only one observation.");
Oracles/UniswapV2PriceOracle.sol:158:            "UniswapV2PriceOracle: Bad TWAP time."
CErc20.sol:136:     require(address(token) != underlying, "CErc20::sweepToken: can not sweep underlying token");
CErc20.sol:234:        require(msg.sender == admin, "only the admin may set the comp-like delegate");
CNft.sol:24:        require(_underlying != address(0), "CNFT: Asset should not be address(0)");
CNft.sol:25:        require(ComptrollerInterface(_comptroller).isComptroller(), "_comptroller is not a Comptroller contract");
CNft.sol:52:                require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
CNft.sol:69:                    require(buyPunkSuccess, "CNFT: Calling buyPunk was unsuccessful");
CNft.sol:93:        require(borrower != liquidator, "CNFT: Liquidator cannot be borrower");
CNft.sol:100:                require(seizeAmounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
CNft.sol:124:                require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
CNft.sol:148:                    require(transferPunkSuccess, "CNFT: Calling transferPunk was unsuccessful");
CNft.sol:204:            revert("CNFT: Use safeBatchTransferFrom instead");
CNft.sol:208:        require(msg.sender == underlying, "CNFT: This contract can only receive the underlying NFT");
CNft.sol:209:        require(operator == address(this), "CNFT: Only the CNFT contract can be the operator");
CNft.sol:279:        require(to != underlying, "CNFT: Cannot make an arbitrary call to underlying NFT");
Comptroller.sol:171:        require(oErr == 0, "exitMarket: getAccountSnapshot failed"); // semi-opaque error code
Comptroller.sol:420:            require(borrowBalance >= repayAmount, "Can not repay more than the total borrow");
Comptroller.sol:702:        require(cNftCollateral == address(nftMarket), "cNFT is from the wrong comptroller");
Comptroller.sol:942:     require(msg.sender == admin || msg.sender == borrowCapGuardian, "only admin or borrow cap guardian can set borrow caps");
Comptroller.sol:960:        require(msg.sender == admin, "only admin can set borrow cap guardian");
Comptroller.sol:995:        require(markets[cAsset].isListed, "cannot pause a market that is not listed");
Comptroller.sol:996:        require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
Comptroller.sol:1009:        require(markets[address(cToken)].isListed, "cannot pause a market that is not listed");
Comptroller.sol:1010:        require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
Comptroller.sol:1019:        require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
Comptroller.sol:1028:        require(msg.sender == pauseGuardian || msg.sender == admin, "only pause guardian and admin can pause");
Comptroller.sol:1037:        require(msg.sender == unitroller.admin(), "only unitroller admin can change brains");
Comptroller.sol:1338:        require(address(nftMarket) == address(0), "nft collateral already initialized");
Comptroller.sol:1339:        require(address(cNft) != address(0), "cannot initialize nft market to the 0 address");
CToken.sol:32:        require(msg.sender == admin, "only admin may initialize the market");
CToken.sol:33:        require(accrualBlockNumber == 0 && borrowIndex == 0, "market may only be initialized once");
CToken.sol:37:        require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");
CToken.sol:49:        require(err == uint(Error.NO_ERROR), "setting interest rate model failed");
CToken.sol:271:        require(err == MathError.NO_ERROR, "borrowBalanceStored: borrowBalanceStoredInternal failed");
CToken.sol:328:        require(err == MathError.NO_ERROR, "exchangeRateStored: exchangeRateStoredInternal failed");
CToken.sol:542:        require(vars.mathErr == MathError.NO_ERROR, "MINT_NEW_TOTAL_SUPPLY_CALCULATION_FAILED");
CToken.sol:545:        require(vars.mathErr == MathError.NO_ERROR, "MINT_NEW_ACCOUNT_BALANCE_CALCULATION_FAILED");
CToken.sol:609:        require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero");
CToken.sol:891:        require(vars.mathErr == MathError.NO_ERROR, "REPAY_BORROW_NEW_ACCOUNT_BORROW_BALANCE_CALCULATION_FAILED");
CToken.sol:894:        require(vars.mathErr == MathError.NO_ERROR, "REPAY_BORROW_NEW_TOTAL_BALANCE_CALCULATION_FAILED");
CToken.sol:986:        require(amountSeizeError == uint(Error.NO_ERROR), "LIQUIDATE_COMPTROLLER_CALCULATE_AMOUNT_SEIZE_FAILED");
CToken.sol:1083:        require(amountSeizeError == uint(Error.NO_ERROR), "LIQUIDATE_COMPTROLLER_CALCULATE_AMOUNT_SEIZE_FAILED");
CToken.sol:1093:        require(seizeTokens == 0, "LIQUIDATE_SEIZE_INCORRECT_NUM_NFTS");
CToken.sol:1433:        require(totalReservesNew <= totalReserves, "reduce reserves unexpected underflow");

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

Oracles/CNftPriceOracle.sol:31:        require(msg.sender == admin, "Sender must be the admin.");
Oracles/CNftPriceOracle.sol:62:        require(
Oracles/CNftPriceOracle.sol:68:            require(
Oracles/CNftPriceOracle.sol:88:        require(
Oracles/UniswapV2PriceOracle.sol:66:                require(
Oracles/UniswapV2PriceOracle.sol:90:                require(
Oracles/UniswapV2PriceOracle.sol:114:                require(
Oracles/UniswapV2PriceOracle.sol:128:        require(length > 0, "UniswapV2PriceOracle: No observations.");
Oracles/UniswapV2PriceOracle.sol:131:            require(length > 1, "UniswapV2PriceOracle: Only one observation.");
Oracles/UniswapV2PriceOracle.sol:135:        require(
Oracles/UniswapV2PriceOracle.sol:149:        require(length > 0, "UniswapV2PriceOracle: No observations.");
Oracles/UniswapV2PriceOracle.sol:152:            require(length > 1, "UniswapV2PriceOracle: Only one observation.");
Oracles/UniswapV2PriceOracle.sol:156:        require(
CNft.sol:24:        require(_underlying != address(0), "CNFT: Asset should not be address(0)");
CNft.sol:25:        require(ComptrollerInterface(_comptroller).isComptroller(), "_comptroller is not a Comptroller contract");
CNft.sol:40:        require(tokenIds.length == amounts.length, "CNFT: id/amounts length mismatch");
CNft.sol:45:        require(mintAllowedResult == 0, "CNFT: Mint is not allowed");
CNft.sol:52:                require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
CNft.sol:66:                    require(checkSuccess && nftOwner == msg.sender, "Not the NFT owner");
CNft.sol:69:                    require(buyPunkSuccess, "CNFT: Calling buyPunk was unsuccessful");
CNft.sol:85:        require(seizeIds.length == seizeAmounts.length, "CNFT: id/amounts length mismatch");
CNft.sol:90:        require(siezeAllowedResult == 0, "CNFT: Seize is not allowed");
CNft.sol:93:        require(borrower != liquidator, "CNFT: Liquidator cannot be borrower");
CNft.sol:100:                require(seizeAmounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
CNft.sol:116:        require(tokenIds.length == amounts.length, "CNFT: id/amounts length mismatch");
CNft.sol:124:                require(amounts[i] == 1, "CNFT: Amounts must be all 1s for non-ERC1155s.");
CNft.sol:127:            require(balanceOf(msg.sender, tokenIds[i]) >= amounts[i], "CNFT: Not enough NFTs to redeem");
CNft.sol:132:        require(redeemAllowedResult == 0, "CNFT: Redeem is not allowed");
CNft.sol:148:                    require(transferPunkSuccess, "CNFT: Calling transferPunk was unsuccessful");
CNft.sol:182:        require(transferAllowedResult == 0, "CNFT: Redeem is not allowed");
CNft.sol:208:        require(msg.sender == underlying, "CNFT: This contract can only receive the underlying NFT");
CNft.sol:209:        require(operator == address(this), "CNFT: Only the CNFT contract can be the operator");
CNft.sol:279:        require(to != underlying, "CNFT: Cannot make an arbitrary call to underlying NFT");

I suggest replacing revert strings with custom errors.

code-423n4 / 2022-05-bunker-findings

Gas Optimizations #118

Copying a full array from storage to memory isn't optimal

Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it

Caching storage values in memory

Unchecking arithmetics operations that can't underflow/overflow

Boolean comparisons

`> 0` is less efficient than `!= 0` for unsigned integers (with proof)

Splitting `require()` statements that use `&&` saves gas

Usage of assert() instead of require()

Amounts should be checked for 0 before calling a transfer

An array's length should be cached to save gas in for-loops

`++i` costs less gas compared to `i++` or `i += 1`

Do not pre-declare variable with default values

Increments can be unchecked

Public functions to external

No need to explicitly initialize variables with default values

Upgrade pragma to at least 0.8.4

`PriceOracleImplementation.cEtherAddress` variable should be immutable

Reduce the size of error messages (Long revert Strings)

Use Custom Errors instead of Revert Strings to save Gas

code-423n4 / 2022-05-bunker-findings

Gas Optimizations #118

Copying a full array from storage to memory isn't optimal

Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it

Caching storage values in memory

Unchecking arithmetics operations that can't underflow/overflow

Boolean comparisons

> 0 is less efficient than != 0 for unsigned integers (with proof)

Splitting require() statements that use && saves gas

Usage of assert() instead of require()

Amounts should be checked for 0 before calling a transfer

An array's length should be cached to save gas in for-loops

++i costs less gas compared to i++ or i += 1

Do not pre-declare variable with default values

Increments can be unchecked

Public functions to external

No need to explicitly initialize variables with default values

Upgrade pragma to at least 0.8.4

PriceOracleImplementation.cEtherAddress variable should be immutable

Reduce the size of error messages (Long revert Strings)

Use Custom Errors instead of Revert Strings to save Gas

`> 0` is less efficient than `!= 0` for unsigned integers (with proof)

Splitting `require()` statements that use `&&` saves gas

`++i` costs less gas compared to `i++` or `i += 1`

`PriceOracleImplementation.cEtherAddress` variable should be immutable