search

code-423n4 / 2022-08-foundation-findings

0 stars 0 forks source link

Gas Optimizations #60

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1. not using the named return variables when a function returns, wastes deployment gas

2. can make the variable outside the loop to save gas

3. it costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

4. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for-loop and while-loops

5. require()/revert() strings longer than 32 bytes cost extra gas

6. use custom errors rather than revert()/require() strings to save deployment gas

https://blog.soliditylang.org/2021/04/21/custom-errors/

basically most requires and reverts use strings and need to be changed

7. using calldata instead of memory for read-only arguments in external functions saves gas

8. using bools for storage incurs overhead

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

9. 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. https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

10. expressions for constant values such as a call to keccak256(), should use immutable rather than constant

11. using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it’s used, and not adding another entry to the method ID table

12. <array>.length should not be looked up in every loop of a for-loop

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

HardlyDifficult commented 2 years ago

Good report, very easy to follow.

  1. not using the named return variables when a function returns, wastes deployment gas

Agree for code consistency with other parts of our code. And this is a thorough list. Saves 0.013 bytes on the bytecode size in total.

  1. can make the variable outside the loop to save gas

This technique is new to me.. but it makes sense that it could make a difference.

I tested it with royalties where 5 recipients were paid: Before: // 221331 · 230960 · 225544 After: // 221316 · 230945 · 225529 Savings: 15 gas

And when royalties are just 1 recipient (i.e. loop is not entered): Before: // 129153 · 136830 · 132512 After: // 129157 · 136834 · 132516 Cost: 4 gas

Since the vast majority of sales use just 1 recipient, and that it does complicate the code a tiny bit - I won't be making the change here.

However for your other example, that loop is always run for 20 iterations -- seems like an easy win. Will be making that change.

  1. it costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Invalid. This optimization technique is no longer applicable with the current version of Solidity.

++i/i++ should be unchecked{++i}

Forgot example links for this one - but I know what you mean from the other submissions :) getFeesAndRecipients is a read only function not intended to be used on-chain, but as a best practice we will add unchecked there as well.

  1. require()/revert() strings longer than 32 bytes

Agree but won't fix. We use up to 64 bytes, aiming to respect the incremental cost but 32 bytes is a bit too short to provide descriptive error messages for our users.

  1. use custom errors rather than revert()/require() strings to save deployment gas

Agree but won't fix at this time. We use these in the market but not in collections. Unfortunately custom errors are still not as good of an experience for users (e.g. on etherscan). We used them in the market originally because we were nearing the max contract size limit and this was a good way to reduce the bytecode. We'll consider this in the future as tooling continues to improve.

  1. using calldata instead of memory for read-only arguments in external functions saves gas

Updated startsWith, NFTCollection, and the AddressLibrary & call Invalid for capLength due to how that data is sourced. Same for replaceAtIf.

  1. using bools for storage incurs overhead

Valid for cidToMinted, saving ~200 gas. Not seeing any benefit for assumePrimarySale, potentially because it's an immutable variable.

Not changing the return values to keep the ABI clean, particularly supportsInterface for example since that would be a violation of the standard.

  1. usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

Potentially valid but won't fix. This is uint32 to allow packing storage for a significant gas savings. Accepting the input as uint256 would incur additional overhead in checking for overflow before saving these values. Other inputs are logically limited to the size exposed, huge values cannot be used. Accepting the input as uint256 would incur additional overhead in checking for overflow before using these values.

  1. expressions for constant values such as a call to keccak256(), should use immutable rather than constant

While it seems this should help, changing to immutable shows a regression in our test suite. Gas costs go up by ~50 gas.

  1. using private rather than public for constants, saves gas

Agree but won't fix. For ease of use and consistency we will continue to expose some constants publicly.

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

May be theoretically valid, but won't fix. I tested this: gas-reporter and our gas-stories suite is reporting a small regression using this technique. It also hurts readability a bit so we wouldn't want to include it unless it was a clear win.