bunker.finance contest - joestakey's results

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparisons with zero for unsigned integers
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Prefix increments
• Require instead of AND
• unchecked arithmetic
• unnecessary computation

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances include:

CNft.sol

scope: mint()

• is1155 is read (1 + length) times: number of reads depending on tokenIds.length as it is in a for loop

CNft.sol:50
CNft.sol:51

• underlying is read (1 + 2*length) times (or (1 + length) times, depending on the if(isPunk)): number of reads depending on tokenIds.length as it is in a for loop

CNft.sol:58
CNft.sol:64
CNft.sol:68
CNft.sol:73

scope: redeem()

• is1155 is read (1 + length) times: number of reads depending on tokenIds.length as it is in a for loop

CNft.sol:123
CNft.sol:140

• underlying is read (1 + 2*length) times (or (1 + length) times, depending on the if(isPunk)): number of reads depending on tokenIds.length as it is in a for loop

CNft.sol:141
CNft.sol:147
CNft.sol:152

Comptroller.sol

scope: getHypotheticalAccountLiquidityInternal()

• oracle is read (1 + assets.length) times: number of reads depending on assets.length as it is in a for loop

Comptroller.sol:603

• nftMarket is read 5 or 6 times(depending on condition line 642):

Comptroller.sol:630
Comptroller.sol:633
Comptroller.sol:638
Comptroller.sol:641
Comptroller.sol:642
Comptroller.sol:645

scope: liquidateCalculateSeizeTokens()

• oracle is read twice:

Comptroller.sol:667
Comptroller.sol:668

scope: liquidateCalculateSeizeNfts()

• oracle is read twice:

Comptroller.sol:702
Comptroller.sol:706

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

CNft.sol

scope: initialize()

CNft.sol:18: string memory _uri

scope: executeCall()

CNft.sol:251: bytes memory data

ERC1155Enumerable.sol

scope: __ERC1155Enumerable_init()

ERC1155Enumerable.sol:26: string memory uri_

scope: _beforeTokenTransfer()

ERC1155Enumerable.sol:39: uint256[] memory ids
ERC1155Enumerable.sol:40: uint256[] memory amounts
ERC1155Enumerable.sol:41: bytes memory data

CToken.sol

scope: liquidateBorrowNftFresh()

CToken.sol:1041: uint[] memory seizeIds
CToken.sol:1041: uint[] memory seizeAmounts

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparisons with zero for unsigned integers

IMPACT

>0 is less gas efficient than != 0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes here

PROOF OF CONCEPT

Instances include:

CNftPriceOracle.sol

CNftPriceOracle.sol:63

UniswapV2PriceOracle.sol

UniswapV2PriceOracle.sol:67
UniswapV2PriceOracle.sol:67
UniswapV2PriceOracle.sol:91
UniswapV2PriceOracle.sol:115
UniswapV2PriceOracle.sol:128
UniswapV2PriceOracle.sol:149

TOOLS USED

Manual Analysis

MITIGATION

Replace > 0 with != 0

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

CNft.sol

CNft.sol:127

PriceOracleImplementation.sol

PriceOracleImplementation.sol:30

UniswapV2PriceOracle.sol

UniswapV2PriceOracle.sol:27
UniswapV2PriceOracle.sol:136
UniswapV2PriceOracle.sol:157

Comptroller.sol

Comptroller.sol:420
Comptroller.sol:1255

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

PriceOracleImplementation.sol

PriceOracleImplementation.sol:12 cEtherAddress = _cEtherAddress

CNftPriceOracle.sol

CNftPriceOracle.sol:48 admin = _admin

TOOLS USED

Manual Analysis

MITIGATION

Hardcode state variables with their value instead of writing them during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

CNft.sol

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:132: 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");

CNftPriceOracle.sol

CNftPriceOracle.sol:62: require(
            cNfts.length > 0 && cNfts.length == nftxTokens.length,
            "CNftPriceOracle: `cNfts` and `nftxTokens` must have nonzero, equal lengths."
        );
CNftPriceOracle.sol:68: require(
                underlyingNftxTokenAddress[underlying] == address(0),
                "CNftPriceOracle: Cannot overwrite existing address mappings."
            );
CNftPriceOracle.sol:88: require(
            nftxToken != address(0),
            "CNftPriceOracle: No NFTX token for cNFT."
        );

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in CNft.sol:

Replace

require(_underlying != address(0), "CNFT: Asset should not be address(0)");

with

if (_underlying == address(0)) {
		revert IsAddressZero();
}

and define the custom error in the contract

error IsAddressZero();

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

CNft.sol

CNft.sol:49: uint256 totalAmount = 0;
CNft.sol:97: uint256 totalAmount = 0;
CNft.sol:119: uint256 totalAmount = 0;

CNftPriceOracle.sol

CNftPriceOracle.sol:66: uint256 i = 0;

UniswapV2PriceOracle.sol

UniswapV2PriceOracle.sol:41: uint256 numberUpdated = 0;
UniswapV2PriceOracle.sol:42: uint256 i = 0;

Comptroller.sol

Comptroller.sol:119: uint256 i = 0;
Comptroller.sol:199: uint256 i = 0;
Comptroller.sol:591: uint256 i = 0;
Comptroller.sol:928: uint256 i = 0;
Comptroller.sol:949: uint256 i = 0;
Comptroller.sol:1223: uint256 i = 0;
Comptroller.sol:1229: uint256 j = 0;
Comptroller.sol:1235: uint256 j = 0;
Comptroller.sol:1240: uint256 j = 0;

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

Comptroller.sol

Comptroller.sol:119
Comptroller.sol:199
Comptroller.sol:591
Comptroller.sol:928
Comptroller.sol:949
Comptroller.sol:1223
Comptroller.sol:1235
Comptroller.sol:1240

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

CNft.sol

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

CNftPriceOracle.sol

CNftPriceOracle:63: require(
            cNfts.length > 0 && cNfts.length == nftxTokens.length,
            "CNftPriceOracle: `cNfts` and `nftxTokens` must have nonzero, equal lengths."
        );

UniswapV2PriceOracle.sol

UniswapV2PriceOracle:67: require(
                    reserve0 > 0 && reserve1 > 0,
                    "UniswapV2PriceOracle: Division by zero."
                );

TOOLS USED

Manual Analysis

MITIGATION

Breakdown the single require statement in multiple ones

For instance:

-require(checkSuccess && nftOwner == msg.sender, "Not the NFT owner");
+require(checkSuccess);
+require(nftOwner == msg.sender, "Not the NFT owner");

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

Comptroller.sol

Comptroller.sol:651: vars.sumCollateral is larger than vars.sumBorrowPlusEffects (condition line 650), underflow check unnecessary
Comptroller.sol:653: vars.sumBorrowPlusEffects is larger than vars.sumCollateral (condition line 650), underflow check unnecessary

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

When emitting an event that includes a new and an old value, it is cheaper in gas to avoid caching the old value in memory. Instead, emit the event, then save the new value in storage. There are several functions affected by these problems in Comptroller.sol

PROOF OF CONCEPT

Instances include:

Comptroller.sol

Comptroller.sol:748-754
Comptroller.sol:786-788
Comptroller.sol:826-830
Comptroller.sol:867-871
Comptroller.sol:889-895
Comptroller.sol:963-969
Comptroller.sol:983-989

TOOLS USED

Manual Analysis

MITIGATION

For instance in _setCloseFactor():

uint oldCloseFactorMantissa = closeFactorMantissa;
closeFactorMantissa = newCloseFactorMantissa;
emit NewCloseFactor(oldCloseFactorMantissa, closeFactorMantissa);

with

emit NewCloseFactor(closeFactorMantissa, newCloseFactorMantissa);
closeFactorMantissa = newCloseFactorMantissa;