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

2 stars 0 forks source link

Gas Optimizations #265

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas Report

Table of Contents

Check non zero values can avoid external call

IMPACT

when calling ERC20 transfer functions, it is good to check the amount is non-zero, to avoid an unnecessary function call

PROOF OF CONCEPT

Instances include:

Cally.sol

Cally.sol:158: function createVault(
        uint256 tokenIdOrAmount) //if it is an ERC20 token, it does not make sense to create a vault for 0 tokens.
Cally.sol:200 : ERC20(vault.token).safeTransferFrom(msg.sender, address(this), vault.tokenIdOrAmount);
Cally.sol:296 : ERC20(vault.token).safeTransfer(msg.sender, vault.tokenIdOrAmount);

TOOLS USED

Manual Analysis

MITIGATION

check that tokenIdOrAmount != 0 before these calls. For createVault() add it only in the case of an ERC20 (some ERC721 may have a token with an id of 0.)

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:

Cally.sol

Cally.sol:170

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:

Cally.sol

Cally.sol:224: require(msg.value >= premium, "Incorrect ETH amount sent");
Cally.sol:228: require(block.timestamp >= auctionStartTimestamp, "Auction not started");

TOOLS USED

Manual Analysis

MITIGATION

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

example:

-block.timestamp >= auctionStartTimestamp
+block.timestamp > auctionStartTimestamp - 1

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

example:

-block.timestamp >= auctionStartTimestamp
+block.timestamp > auctionStartTimestamp

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:

Cally.sol

Cally.sol:167
Cally.sol:168
Cally.sol:169
Cally.sol:170
Cally.sol:171
Cally.sol:211
Cally.sol:214
Cally.sol:217
Cally.sol:220
Cally.sol:224
Cally.sol:228
Cally.sol:260
Cally.sol:263
Cally.sol:269
Cally.sol:272
Cally.sol:304
Cally.sol:307
Cally.sol:320
Cally.sol:323
Cally.sol:328
Cally.sol:329
Cally.sol:330
Cally.sol:353
Cally.sol:354
Cally.sol:436
Cally.sol:437
Cally.sol:438
Cally.sol:456

CallyNFT.sol

CallyNFT.sol:15
CallyNFT.sol:16
CallyNFT.sol:36
CallyNFT.sol:42

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in Cally.sol:

Replace

require(_ownerOf[tokenId] != address(0), "URI query for NOT_MINTED token");

with

if (_ownerOf[tokenId] == address(0)) {
        revert IsNotMinted(tokenId);
}

and define the custom error in the contract

error IsNotMinted(uint256 _tokenId_);

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:

Cally.sol

Cally.sol:94: uint256 public feeRate = 0;
Cally.sol:95: uint256 public protocolUnclaimedFees = 0;
Cally.sol:282: uint256 fee = 0;

CallyNft.sol

CallyNft.sol:244: uint256 i = 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:

CallyNft.sol

CallyNft.sol:244: i++

TOOLS USED

Manual Analysis

MITIGATION

change i++ to ++i.

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:

CallyNft.sol

CallyNft.sol:244: i bounded by data.length, which is bounded as data.length = 20, overflow check unnecessary

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

If a value is known, it is more efficient to hardcode it than read it from memory

PROOF OF CONCEPT

Instances include:

CallyNft.sol

CallyNft.sol:241
CallyNft.sol:244
account is an address, so data.length == 20

TOOLS USED

Manual Analysis

MITIGATION

Replace

data.length

with

20