Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #132

Open
code423n4 opened this issue May 13, 2022 · 1 comment
Open

Gas Optimizations #132

code423n4 opened this issue May 13, 2022 · 1 comment

Comments

@code423n4
Copy link
Contributor

Gas Optimizations

Use != 0 instead of > 0 for unsigned integers

Since the variable being compared is uint, the value will never be below 0, thus checking if != 0 is sufficient and more gas efficient.

I suggest changing from > 0 to != 0 in these lines:

Cally.sol:170           require(durationDays > 0, "durationDays too small");
Cally.sol:283           if (feeRate > 0) {

Unecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type (0 for uint256). Thus, explicitly initializing a variable with its default value costs unnecesary gas.

I suggest changing Cally.sol:94-95 from:

    uint256 public feeRate = 0;
    uint256 public protocolUnclaimedFees = 0;

to

    uint256 public feeRate;
    uint256 public protocolUnclaimedFees;

Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

I suggest changing the visibility of the following from public to internal or private:

Cally.sol:87            uint32 public constant AUCTION_DURATION = 24 hours;

Boolean comparisons

Comparing to a constant (true or false) is a bit more expensive than directly checking the boolean value.

Considering changing the following lines from var == false to !var:

Cally.sol:217           require(vault.isExercised == false, "Vault already exercised");
Cally.sol:220           require(vault.isWithdrawing == false, "Vault is being withdrawn");
Cally.sol:328           require(vault.isExercised == false, "Vault already exercised");

Unchecking arithmetic operations that cannot underflow/overflow

Since Solidity v0.8.0, all arithmetic operations come with implicit overflow and underflow checks. In some instances, an overflow/underflow is impossible, such as:

  • A check is in place before the arithmetic operation
  • The value realistically cannot overflow, such as amount of ETH sent

As such, gas can be saved by using an unchecked block to remove the implicit checks:

    unchecked { a += b; }

Below are instances that can be left unchecked:

Cally.sol:188           vaultIndex += 2;
Cally.sol:245           optionId = vaultId + 1;
Cally.sol:250           ethBalance[beneficiary] += msg.value;
Cally.sol:285           protocolUnclaimedFees += fee;
Cally.sol:333           uint256 optionId = vaultId + 1;

Unnecessary definition of variables in Cally.sol

Some variables are defined even though they are only referenced once in their respective functions. Not defining these variables can help to reduce gas cost and contract size.

  • Cally.sol:223-224:
    // Original code
    uint256 premium = getPremium(vaultId);
    require(msg.value >= premium, "Incorrect ETH amount sent");

    // Change to:
    require(msg.value >= getPremium(vaultId), "Incorrect ETH amount sent");
  • Cally.sol:227-228:
    // Original code
    uint32 auctionStartTimestamp = vault.currentExpiration;
    require(block.timestamp >= auctionStartTimestamp, "Auction not started");

    // Change to:
    require(block.timestamp >= vault.currentExpiration, "Auction not started");
  • Cally.sol:333-334:
    // Original code
    uint256 optionId = vaultId + 1;
    _burn(optionId);

    // Change to:
    _burn(vaultId + 1);
  • Cally.sol:395-396:
    // Original code
    Vault memory vault = _vaults[vaultId];
    return premiumOptions[vault.premiumIndex];

    // Change to:
    return premiumOptions[_vaults[vaultId].premiumIndex];
  • Cally.sol:444-445:
    // Original code
    bool isVaultToken = id % 2 != 0;
    if (isVaultToken) {

    // Change to:
    if (id % 2 != 0) {

buyOption() can be optimized when an invalid vaultId is provided

Cally.sol:208-214:

    Vault memory vault = _vaults[vaultId];

    // vaultId should always be odd
    require(vaultId % 2 != 0, "Not vault type");
    
    // check vault exists
    require(ownerOf(vaultId) != address(0), "Vault does not exist");

In buyOption(), the function should check if vaultId is valid before fetching vault from storage. This would reduce gas cost when an invalid vaultId is provided.

Consider fetching vault from storage after parameter validation:

    // vaultId should always be odd
    require(vaultId % 2 != 0, "Not vault type");

    // check vault exists
    require(ownerOf(vaultId) != address(0), "Vault does not exist");

    Vault memory vault = _vaults[vaultId];

exercise() can be optimized

In Cally.sol:278-279, _vaults[vaultId].isExercised can be updated directly instead of using vault:

    // Original code
    vault.isExercised = true;
    _vaults[vaultId] = vault;

    // Change to:
    _vaults[vaultId].isExercised = true;

Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

  • Cheaper deployment cost
  • Lower runtime cost upon revert

Taken from Custom Errors in Solidity:

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 can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

CallyNft.sol:15          require(to != address(0), "INVALID_RECIPIENT");
CallyNft.sol:16          require(_ownerOf[id] == address(0), "ALREADY_MINTED");
CallyNft.sol:36          require(owner != address(0), "ZERO_ADDRESS");
CallyNft.sol:42          require(to != address(0), "INVALID_RECIPIENT");
Cally.sol:167           require(premiumIndex < premiumOptions.length, "Invalid premium index");
Cally.sol:168           require(dutchAuctionStartingStrikeIndex < strikeOptions.length, "Invalid strike index");
Cally.sol:169           require(dutchAuctionReserveStrike < strikeOptions[dutchAuctionStartingStrikeIndex], "Reserve strike too small");
Cally.sol:170       require(durationDays > 0, "durationDays too small");
Cally.sol:171       require(tokenType == TokenType.ERC721 || tokenType == TokenType.ERC20, "Invalid token type");
Cally.sol:211       require(vaultId % 2 != 0, "Not vault type");
Cally.sol:214       require(ownerOf(vaultId) != address(0), "Vault does not exist");
Cally.sol:217       require(vault.isExercised == false, "Vault already exercised");
Cally.sol:220       require(vault.isWithdrawing == false, "Vault is being withdrawn");
Cally.sol:224       require(msg.value >= premium, "Incorrect ETH amount sent");
Cally.sol:228       require(block.timestamp >= auctionStartTimestamp, "Auction not started");
Cally.sol:260       require(optionId % 2 == 0, "Not option type");\
Cally.sol:263       require(msg.sender == ownerOf(optionId), "You are not the owner");
Cally.sol:269       require(block.timestamp < vault.currentExpiration, "Option has expired");
Cally.sol:272       require(msg.value == vault.currentStrike, "Incorrect ETH sent for strike");
Cally.sol:304       require(vaultId % 2 != 0, "Not vault type");
Cally.sol:307       require(msg.sender == ownerOf(vaultId), "You are not the owner");
Cally.sol:320       require(vaultId % 2 != 0, "Not vault type");
Cally.sol:323       require(msg.sender == ownerOf(vaultId), "You are not the owner");
Cally.sol:328       require(vault.isExercised == false, "Vault already exercised");
Cally.sol:329       require(vault.isWithdrawing, "Vault not in withdrawable state");
Cally.sol:330       require(block.timestamp > vault.currentExpiration, "Option still active");
Cally.sol:353       require(vaultId % 2 != 0, "Not vault type");
Cally.sol:354       require(msg.sender == ownerOf(vaultId), "Not owner");
Cally.sol:436       require(from == _ownerOf[id], "WRONG_FROM");
Cally.sol:437       require(to != address(0), "INVALID_RECIPIENT");

Cally.sol:438-441
   require(
        msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
        "NOT_AUTHORIZED"
    );

Cally.sol:456           require(_ownerOf[tokenId] != address(0), "URI query for NOT_MINTED token");
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels May 13, 2022
code423n4 added a commit that referenced this issue May 13, 2022
@outdoteth
Copy link
Collaborator

high quality report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants