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.
Proof of Concept
CEther.sol::175 => bytes memory fullMessage = new bytes(bytes(message).length + 5);
CEther.sol::178 => for (i = 0; i < bytes(message).length; i++) {
CNft.sol::176 => for (uint256 i; i < vars.length; ++i) {
CNftPriceOracle.sol::66 => for (uint256 i = 0; i < cNfts.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) {
UniswapV2PriceOracle.sol::42 => for (uint256 i = 0; i < pairs.length; ++i) {
Recommendation
Store the array’s length in a variable before the for-loop.
2. Use != 0 instead of > 0 for Unsigned Integer Comparison.
Impact
!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.
3. Reduce the size of error messages (Long revert Strings).
Impact
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.
Proof of Concept
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::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::64 => "CNftPriceOracle: `cNfts` and `nftxTokens` must have nonzero, equal lengths."
CNftPriceOracle.sol::70 => "CNftPriceOracle: Cannot overwrite existing address mappings."
CNftPriceOracle.sol::90 => "CNftPriceOracle: No NFTX token for cNFT."
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");
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");
UniswapV2PriceOracle.sol::68 => "UniswapV2PriceOracle: Division by zero."
UniswapV2PriceOracle.sol::92 => "UniswapV2PriceOracle: Division by zero."
UniswapV2PriceOracle.sol::116 => "UniswapV2PriceOracle: Division by zero."
UniswapV2PriceOracle.sol::128 => require(length > 0, "UniswapV2PriceOracle: No observations.");
UniswapV2PriceOracle.sol::131 => require(length > 1, "UniswapV2PriceOracle: Only one observation.");
UniswapV2PriceOracle.sol::137 => "UniswapV2PriceOracle: Bad TWAP time."
UniswapV2PriceOracle.sol::149 => require(length > 0, "UniswapV2PriceOracle: No observations.");
UniswapV2PriceOracle.sol::152 => require(length > 1, "UniswapV2PriceOracle: Only one observation.");
UniswapV2PriceOracle.sol::158 => "UniswapV2PriceOracle: Bad TWAP time."
Recommendation
Shorten the revert strings to fit in 32 bytes.
4. public Functions can be external.
Impact
public functions that are never called by the contract should be declared external to save gas.
Proof of Concept
_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)
safeTransferFrom(address,address,uint256,uint256,bytes) should be declared external:
- CNft.safeTransferFrom(address,address,uint256,uint256,bytes) (contracts/CNft.sol#190-205)
initialize(string,address,bool,bool,address) should be declared external:
- CNft.initialize(string,address,bool,bool,address) (contracts/CNft.sol#17-33)
onERC721Received(address,address,uint256,bytes) should be declared external:
- CNft.onERC721Received(address,address,uint256,bytes) (contracts/CNft.sol#213-220)
onERC1155Received(address,address,uint256,uint256,bytes) should be declared external:
- CNft.onERC1155Received(address,address,uint256,uint256,bytes) (contracts/CNft.sol#222-230)
onERC1155BatchReceived(address,address,uint256[],uint256[],bytes) should be declared external:
- CNft.onERC1155BatchReceived(address,address,uint256[],uint256[],bytes) (contracts/CNft.sol#232-240)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external
Recommendation
Declare visibility of functions above as external.
5. No need to initialize variables with default values
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
CNft.sol::49 => uint256 totalAmount = 0;
CNft.sol::97 => uint256 totalAmount = 0;
CNft.sol::119 => uint256 totalAmount = 0;
CNftPriceOracle.sol::66 => for (uint256 i = 0; i < cNfts.length; ++i) {
CToken.sol::81 => uint startingAllowance = 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++) {
UniswapV2PriceOracle.sol::41 => uint256 numberUpdated = 0;
UniswapV2PriceOracle.sol::42 => for (uint256 i = 0; i < pairs.length; ++i) {
Recommendation
Remove explicit zero initialization.
6. ++i costs less gas compared to i++ or i += 1
Impact
++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.
Proof of Concept
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++) {
CEther.sol::178 => for (i = 0; i < bytes(message).length; i++) {
Recommendation
Use ++i instead of i++ to increment the value of an uint variable.
Same thing for --i and i--.
Gas
1. Cache Array Length Outside of Loop.
Impact
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.
Proof of Concept
Recommendation
Store the array’s length in a variable before the for-loop.
2. Use
!= 0
instead of> 0
for Unsigned Integer Comparison.Impact
!= 0
is cheapear than> 0
when comparing unsigned integers in require statements.Proof of Concept
Recommendation
Use
!= 0
instead of> 0
.3. Reduce the size of error messages (Long revert Strings).
Impact
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.
Proof of Concept
Recommendation
Shorten the revert strings to fit in 32 bytes.
4.
public
Functions can beexternal
.Impact
public
functions that are never called by the contract should be declaredexternal
to save gas.Proof of Concept
Recommendation
Declare visibility of functions above as
external
.5. No need to initialize variables with default values
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
Recommendation
Remove explicit zero initialization.
6.
++i
costs less gas compared toi++
ori += 1
Impact
++i
costs less gas compared toi++
ori += 1
for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.Proof of Concept
Recommendation
Use
++i
instead ofi++
to increment the value of an uint variable. Same thing for--i
andi--
.