search

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

0 stars 0 forks source link

Gas Optimizations #120

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Gas optimisation and QA

  1. Variables initialized to default values waste gas
  1. Array length is calculated inside the loop

This extra calculation can be avoided by caching the length of the array outside of the loop in a Variable

contracts/PercentSplitETH.sol: lines 115, 135, 227, 261, 320 contracts/mixins/shared/MarketFees.sol: lines 126, 198, 503

  1. The inequality check (!=) is less costly than the greater than (>) check. So for unsigned integers, prefer using != 0, rather than > 0

contracts/NFTDropCollection.sol: lines 88, 130, 131 contracts/mixins/shared/MarketFees.sol: lines 244 contracts/mixins/shared/OZERC165Checker.sol: lines 36

  1. Strings used in revert/require statements should be less than 32 bytes in UTF-8 encoding. Lengths of strings can be quickly checked in https://mothereff.in/byte-counter

contracts/NFTCollection.sol: lines 158, 263, 264, 268, 327 contracts/NFTCollectionFactory.sol: lines 173, 182, 203, 227, 262 contracts/NFTDropCollection.sol: lines 88, 93, 130, 131, 172, 179, 238 contracts/PercentSplitETH.sol: lines 121, 136, 148 contracts/libraries/AddressLibrary.sol: lines 31 contracts/mixins/collections/SequentialMintCollection.sol: lines 58, 63, 74, 87, 88, 89 contracts/mixins/roles/AdminRole.sol: lines 19 contracts/mixins/roles/MinterRole.sol: lines 22 contracts/mixins/shared/ContractFactory.sol: lines 22 contracts/mixins/shared/ContractFactory.sol: lines 31 contracts/mixins/treasury/AdminRole.sol: lines 20 contracts/mixins/treasury/OZAccessControlUpgradeable.sol: lines 137, 152

Low severity issue

  1. Unsafe ERC20 Operation Function transfer should be replaced by call function Consensys SC security best practices

contracts/PercentSplitETH.sol: lines 236, 245

HardlyDifficult commented 2 years ago

Don't initialize variables with default values.

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

Cache Array Length Outside of 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.

Use != 0 instead of > 0

Invalid. We tested the recommendation and got the following results:

createNFTDropCollection gas reporter results:
  using > 0 (current):
    - 319246  ·     319578  ·      319361
  using != 0 (recommendation):
    -  319252  ·     319584  ·      319367
  impact: +6 gas

Use short error messages

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.

Unsafe ERC20 Operation

This should have been submitted as a QA report