search

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

0 stars 0 forks source link

Gas Optimizations #462

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Details

1. Post-increment/decrement cost more gas then pre-increment/decrement

Description

++I (--I) cost less gas than I++ (I--) especially in a loop.

Proof of concept

contract TestPost {
    function testPost() public {
        uint256 i;
        i++;
    }
}
contract TestPre {
    function testPre() public {
        uint256 i;
        ++i;
    }
}

https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/Pages.sol#L251) https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/token/PagesERC721.sol#L117) https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/token/PagesERC721.sol#L181) https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/utils/GobblerReserve.sol#L37) https://github.com/transmissions11/solmate/blob/bff24e835192470ed38bf15dbed6084c2d723ace/src/tokens/ERC721.sol#L101) https://github.com/transmissions11/solmate/blob/bff24e835192470ed38bf15dbed6084c2d723ace/src/tokens/ERC721.sol#L164)

2. Array length should not be looked up in every loop of a for-loop

Description

Storage array length checks incur an extra Gwarmaccess (100 gas) per loop. Store the array length in a variable and use it in the for loop helps to save gas

Lines in the code

GobblersERC1155B.sol#L114 GobblerReserve.sol#L37

3. I = I + (-) X is cheaper in gas cost than I += (-=) X

Description

This is especially with state variables. In the following example I'm trying to demostrate the save gas in a loop of 10 iterations.

Proof of concept

contract TestA {
    uint256 i;
    function Test () public {

        for(;i<10;){
            i += 1;
        }
    }

}

contract TestB {
    uint256 i;
    function Test () public {

        for(;i<10;){
            i = i + 1;
        }
    }
}

Lines in the code

ArtGobblers.sol#L439 ArtGobblers.sol#L456 ArtGobblers.sol#L464 ArtGobblers.sol#L662 ArtGobblers.sol#L844 ArtGobblers.sol#L905 ArtGobblers.sol#L906 ArtGobblers.sol#L912 ArtGobblers.sol#L913 Pages.sol#L244 GobblersERC721.sol#L184 SignedWadMath.sol#L203 SignedWadMath.sol#L205

4. Use custom errors rather than REVERT()/REQUIRE() strings to save deployment gas

Description

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

Lines in the code

ArtGobblers.sol#L437 ArtGobblers.sol#L696 ArtGobblers.sol#L705 ArtGobblers.sol#L711 ArtGobblers.sol#L885 ArtGobblers.sol#L887 ArtGobblers.sol#L889 Pages.sol#L266 GobblersERC1155B.sol#L107 GobblersERC1155B.sol#L149 GobblersERC1155B.sol#L185 GobblersERC721.sol#L62 GobblersERC721.sol#L66 GobblersERC721.sol#L95 GobblersERC721.sol#L121 GobblersERC721.sol#L137 PagesERC721.sol#L55 PagesERC721.sol#L59 PagesERC721.sol#L85 PagesERC721.sol#L103 PagesERC721.sol#L105 PagesERC721.sol#L107 PagesERC721.sol#L135 PagesERC721.sol#L151 VRGDA.sol#L32 ERC721.sol#L36 ERC721.sol#L40 ERC721.sol#L69 ERC721.sol#L87 ERC721.sol#L89 ERC721.sol#L91 ERC721.sol#L118 ERC721.sol#L134 ERC721.sol#L158 ERC721.sol#L160 ERC721.sol#L175 ERC721.sol#L196 ERC721.sol#L211 SignedWadMath.sol#L90 SignedWadMath.sol#L142 Owned.sol#L20

5. Using private rather than public for constants, saves gas

Description

If needed, the value can be read from the verified contract source code. 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

Lines in the code

ArtGobblers.sol#L112 ArtGobblers.sol#L115 ArtGobblers.sol#L118 ArtGobblers.sol#L122 ArtGobblers.sol#L126 ArtGobblers.sol#L177 ArtGobblers.sol#L180 ArtGobblers.sol#L184 DeployRinkeby.s.sol#L13 DeployRinkeby.s.sol#L14 DeployRinkeby.s.sol#L15

6. Usage of uints/ints smaller than 32 Bytes (256 bits) incurs overhead

Description

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. Use a larger size then downcast where needed.

Lines in the code

ArtGobblers.sol#L159 ArtGobblers.sol#L167 ArtGobblers.sol#L189 ArtGobblers.sol#L191 ArtGobblers.sol#L204 ArtGobblers.sol#L206 ArtGobblers.sol#L208 ArtGobblers.sol#L210 ArtGobblers.sol#L615 ArtGobblers.sol#L623 ArtGobblers.sol#L899 Pages.sol#L107 Pages.sol#L114 GobblersERC1155B.sol#L48 GobblersERC1155B.sol#L50 GobblersERC721.sol#L38 GobblersERC721.sol#L40 GobblersERC721.sol#L49 GobblersERC721.sol#L51 GobblersERC721.sol#L53 GobblersERC721.sol#L55

7. Use a more recent version of solidity

Description

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value. https://github.com/ethereum/solidity/releases

Lines in the code

ArtGobblers.sol#L2 Goo.sol#L2 Pages.sol#L2 GobblersERC1155B.sol#L2 GobblersERC721.sol#L2 PagesERC721.sol#L2 ChainlinkV1RandProvider.sol#L2 GobblerReserve.sol#L2 DeployBase.s.sol#L2 DeployRinkeby.s.sol#L2 LogisticToLinearVRGDA.sol#L2 LogisticVRGDA.sol#L2 VRGDA.sol#L2 LinearVRGDA.sol#L2 LibGOO.sol#L2 FixedPointMathLib.sol#L2 ERC721.sol#L2 SignedWadMath.sol#L2 MerkleProofLib.sol#L2 LibString.sol#L2 Owned.sol#L2

8. Using storage instead of memory for structs/arrays

Description

When retrieving data from a memory location, assigning the data to a memory variable causes all fields of the struct/array to be read from memory, resulting in a Gcoldsload (2100 gas) for each field of the struct/array. When reading fields from new memory variables, they cause an extra MLOAD instead of a cheap stack read. Rather than declaring a variable with the memory keyword, it is much cheaper to declare a variable with the storage keyword and cache all fields that need to be read again in a stack variable, because the fields actually read will only result in a Gcoldsload. The only case where the entire struct/array is read into a memory variable is when the entire struct/array is returned by a function, passed to a function that needs memory, or when the array/struct is read from another store array/struc

Lines in the code

GobblersERC1155B.sol#L168 GobblersERC1155B.sol#L169

9. Using bools for storage incurs overhead

Description

Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from false to true , after having been true in the past.

Lines in the code

ArtGobblers.sol#L149 ArtGobblers.sol#L212 GobblersERC1155B.sol#L37 GobblersERC721.sol#L77 PagesERC721.sol#L70 ERC721.sol#L51

10. Multiplication/division by two should use bit shifting

Description

* 2 is equivalent to << 1 and / 2 is the same as >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas ### Lines in the code [ArtGobblers.sol#L462](https://github.com/code-423n4/2022-09-artgobblers/blob/d2087c5a8a6a4f1b9784520e7fe75afa3a9cbdbe/src/ArtGobblers.sol#L462)
GalloDaSballo commented 1 year ago

50 gas

ajara87 commented 1 year ago

how do you calculate 50 gas? I'm adding PoC that demostrate that it's saving more than 50 gas

GalloDaSballo commented 1 year ago

The sponsor offered fully benchmarked code, you did not provide tests and benchmark on the in-scope codebase, for those reasons I dismissed your estimates and used my own.

Check similar reports as yours and see how they've been rated consistently and notice how I gave accurate estimates exclusively to submissions that provided benchmarks based on the in-scope codebase.

TL;DR Send me Forge Snapshots of the codebase in scope and I'll trust your benchmarks