search

code-423n4 / 2022-09-artgobblers-findings

0 stars 0 forks source link

Gas Optimizations #397

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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.

Instances number of this issue: 7

// src/ArtGobblers.sol
432:    for (uint256 i = 0; i < cost; ++i) {
592:    for (uint256 i = 0; i < numGobblers; ++i) {

// src/Pages.sol
251:    for (uint256 i = 0; i < numPages; i++) _mint(community, ++lastMintedPageId);

// src/utils/GobblerReserve.sol
37:     for (uint256 i = 0; i < ids.length; i++) {

// src/utils/token/GobblersERC721.sol
186:    for (uint256 i = 0; i < amount; ++i) {

// rc/utils/token/GobblersERC1155B.sol
114:    for (uint256 i = 0; i < owners.length; ++i) {
173:    for (uint256 i = 0; i < amount; ++i) {

The demo of the loop gas comparison can be seen here.

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances number of this issue: 1

// src/utils/GobblerReserve.sol
37:     for (uint256 i = 0; i < ids.length; i++) {

For-loop length could be cached

The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here. Instances number of this issue: 1

// src/utils/GobblerReserve.sol
37:     for (uint256 i = 0; i < ids.length; i++) {

Use bytes32 instead of string

Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32.

Instances number of this issue: 3 Bytes32 can be used instead of string in the following:

// src/ArtGobblers.sol
136:    string public UNREVEALED_URI;
139:    string public BASE_URI;

// src/Pages.sol
96:     string public BASE_URI;

Use custom errors rather than revert()/require() strings

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Instances number of this issue: 37

// lib/solmate/src/auth/Owned.sol
20:     require(msg.sender == owner, "UNAUTHORIZED");

// lib/solmate/src/tokens/ERC721.sol
36:     require((owner = _ownerOf[id]) != address(0), "NOT_MINTED");
40:     require(owner != address(0), "ZERO_ADDRESS");
69:     require(msg.sender == owner || isApprovedForAll[owner][msg.sender], "NOT_AUTHORIZED");
87:     require(from == _ownerOf[id], "WRONG_FROM");
89:     require(to != address(0), "INVALID_RECIPIENT");
91-94:
        require(
            msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
            "NOT_AUTHORIZED"
        );
118-123:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
134-139:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
158:    require(to != address(0), "INVALID_RECIPIENT");
160:    require(_ownerOf[id] == address(0), "ALREADY_MINTED");
175:    require(owner != address(0), "NOT_MINTED");
196-201:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
211-216:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );

// lib/solmate/src/utils/SignedWadMath.sol
142:    require(x > 0, "UNDEFINED");

// lib/VRGDAs/src/VRGDA.sol
32:     require(decayConstant < 0, "NON_NEGATIVE_DECAY_CONSTANT");

// src/ArtGobblers.sol
437:    require(getGobblerData[id].owner == msg.sender, "WRONG_FROM");
885:    require(from == getGobblerData[id].owner, "WRONG_FROM");
887:    require(to != address(0), "INVALID_RECIPIENT");
889-892:
        require(
            msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
            "NOT_AUTHORIZED"
        );

// src/utils/token/GobblersERC721.sol
62:     require((owner = getGobblerData[id].owner) != address(0), "NOT_MINTED");
66:     require(owner != address(0), "ZERO_ADDRESS");
95:     require(msg.sender == owner || isApprovedForAll[owner][msg.sender], "NOT_AUTHORIZED");
121-126:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
137-142:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );

// src/utils/token/GobblersERC1155B.sol
107:    require(owners.length == ids.length, "LENGTH_MISMATCH");
149-153:
        require(
            ERC1155TokenReceiver(to).onERC1155Received(msg.sender, address(0), id, 1, data) ==
                ERC1155TokenReceiver.onERC1155Received.selector,
            "UNSAFE_RECIPIENT"
        );
185-189:
        require(
            ERC1155TokenReceiver(to).onERC1155BatchReceived(msg.sender, address(0), ids, amounts, data) ==
                ERC1155TokenReceiver.onERC1155BatchReceived.selector,
            "UNSAFE_RECIPIENT"
        );

src/utils/token/PagesERC721.sol
55:     require((owner = _ownerOf[id]) != address(0), "NOT_MINTED");
59:     require(owner != address(0), "ZERO_ADDRESS");
85:     require(msg.sender == owner || isApprovedForAll(owner, msg.sender), "NOT_AUTHORIZED");
103:    require(from == _ownerOf[id], "WRONG_FROM");
105:    require(to != address(0), "INVALID_RECIPIENT");
107-110:
        require(
            msg.sender == from || isApprovedForAll(from, msg.sender) || msg.sender == getApproved[id],
            "NOT_AUTHORIZED"
        );
135-139:
        require(
            ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
151-155:
        require(
            ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );

// src/Pages.sol
266:    if (pageId == 0 || pageId > currentId) revert("NOT_MINTED");

Duplicated require()/assert() checks could be refactored to a modifier or function

Instances number of this issue: 16

address(0) check: 8

// lib/solmate/src/tokens/ERC721.sol
40:     require(owner != address(0), "ZERO_ADDRESS");
89:     require(to != address(0), "INVALID_RECIPIENT");
158:    require(to != address(0), "INVALID_RECIPIENT");
175:    require(owner != address(0), "NOT_MINTED");

// src/ArtGobblers.sol
887:    require(to != address(0), "INVALID_RECIPIENT");

// src/utils/token/GobblersERC721.sol
66:     require(owner != address(0), "ZERO_ADDRESS");

src/utils/token/PagesERC721.sol
59:     require(owner != address(0), "ZERO_ADDRESS");
105:    require(to != address(0), "INVALID_RECIPIENT");

ERC721TokenReceiver(to).onERC721Received check: 8

// lib/solmate/src/tokens/ERC721.sol
118-123:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
134-139:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
196-201:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
211-216:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, address(0), id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );

// src/utils/token/GobblersERC721.sol
121-126:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
137-142:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
135-139:
        require(
            ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
151-155:
        require(
            ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );

Using bool for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

Instances number of this issue: 1

// src/ArtGobblers.sol
212:    bool waitingForSeed;

Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

Reference: Layout of State Variables in Storage

Instances number of this issue: 20

// src/ArtGobblers.sol
159:    uint128 public numMintedFromGoo;
167:    uint128 public currentNonLegendaryId;
189:    uint128 startPrice;
191:    uint128 numSold;
204:    uint64 randomSeed;
206:    uint64 nextRevealTimestamp;
208:    uint56 lastRevealedId;
210:    uint56 toBeRevealed;
899:    uint32 emissionMultiple = getGobblerData[id].emissionMultiple; // Caching saves gas.

// src/Pages.sol
107:    uint128 public currentId;
114:    uint128 public numMintedForCommunity;

// src/utils/token/GobblersERC721.sol
38:     uint64 idx;
40:     uint32 emissionMultiple;
49:     uint32 gobblersOwned;
51:     uint32 emissionMultiple;
53:     uint128 lastBalance;
55:     uint64 lastTimestamp;

// src/utils/token/GobblersERC1155B.sol
48:     uint64 idx;
50:     uint32 emissionMultiple;

State variables only set in the constructor should be declared immutable

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas). If getters are still desired, '_' can be added to the variable name and the getter can be added manually.

Instances number of this issue: 3 The followings can be changed from public to internal or private:

// src/ArtGobblers.sol
136:    string public UNREVEALED_URI;
139:    string public BASE_URI;

// src/Pages.sol
96:     string public BASE_URI;

Making some constants/immutable as non-public

Changing the visibility from public to private or internal can save gas when a constant isn’t used outside of its contract.

Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

If needed, the value can be read from the verified contract source code.

A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract’s value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don’t get out of sync.

Instances number of this issue: 13 The followings can be changed from public to internal or private:

// src/ArtGobblers.sol
112:    uint256 public constant MAX_SUPPLY = 10000;
115:    uint256 public constant MINTLIST_SUPPLY = 2000;
118:    uint256 public constant LEGENDARY_SUPPLY = 10;
122:    uint256 public constant RESERVED_SUPPLY = (MAX_SUPPLY - MINTLIST_SUPPLY - LEGENDARY_SUPPLY) / 5;
126-129:
        uint256 public constant MAX_MINTABLE = MAX_SUPPLY
            - MINTLIST_SUPPLY
            - LEGENDARY_SUPPLY
            - RESERVED_SUPPLY;
146:    bytes32 public immutable merkleRoot;
156:    uint256 public immutable mintStart;
159:    uint128 public numMintedFromGoo;
177:    uint256 public constant LEGENDARY_GOBBLER_INITIAL_START_PRICE = 69;
180:    uint256 public constant FIRST_LEGENDARY_GOBBLER_ID = MAX_SUPPLY - LEGENDARY_SUPPLY + 1;
184:    uint256 public constant LEGENDARY_AUCTION_INTERVAL = MAX_MINTABLE / (LEGENDARY_SUPPLY + 1);

// lib/VRGDAs/src/VRGDA.sol
17:     int256 public immutable targetPrice;
21:     int256 internal immutable decayConstant;

Some variables can be stored as constants

Instances number of this issue: 6 ERC721TokenReceiver.onERC721Received.selector and ERC1155TokenReceiver.onERC1155BatchReceived.selector can be saved as constants.

// src/utils/token/GobblersERC721.sol
121-126:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
137-142:
        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );

// src/utils/token/PagesERC721.sol
135-139:
            require(
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                    ERC721TokenReceiver.onERC721Received.selector,
                "UNSAFE_RECIPIENT"
            );
151-155:            
            require(
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, data) ==
                    ERC721TokenReceiver.onERC721Received.selector,
                "UNSAFE_RECIPIENT"
            );

// src/utils/token/GobblersERC1155B.sol
149-153:
            require(
                ERC1155TokenReceiver(to).onERC1155Received(msg.sender, address(0), id, 1, data) ==
                    ERC1155TokenReceiver.onERC1155Received.selector,
                "UNSAFE_RECIPIENT"
            );
185-189:
            require(
                ERC1155TokenReceiver(to).onERC1155BatchReceived(msg.sender, address(0), ids, amounts, data) ==
                    ERC1155TokenReceiver.onERC1155BatchReceived.selector,
                "UNSAFE_RECIPIENT"
            );

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

Instances number of this issue: 10

// src/ArtGobblers.sol
439:    burnedMultipleTotal += getGobblerData[id].emissionMultiple;
456:    getUserData[msg.sender].emissionMultiple += uint32(burnedMultipleTotal); // Update multiple.
464:    legendaryGobblerAuctionData.numSold += 1; // Increment the # of legendaries sold.
662:    getUserData[currentIdOwner].emissionMultiple += uint32(newCurrentIdMultiple);
844:    uint256 newNumMintedForReserves = numMintedForReserves += (numGobblersEach << 1);
912:    getUserData[to].emissionMultiple += emissionMultiple;
905:    getUserData[to].gobblersOwned += 1;
            getUserData[from].emissionMultiple -= emissionMultiple;
906:    getUserData[from].gobblersOwned -= 1;

// src/Pages.sol
244:    uint256 newNumMintedForCommunity = numMintedForCommunity += uint128(numPages);

// src/utils/token/GobblersERC721.sol
184:    getUserData[to].gobblersOwned += uint32(amount);
GalloDaSballo commented 2 years ago

6.3k from immutables

Rest 100 gas

212: bool waitingForSeed;

Will not save gas because it's packed in an already dirty slot

6.4k