Foundation Drop contest - 0xkatana's results

Foundation is a web3 destination.

General Information

Platform: Code4rena

Start Date: 11/08/2022

Pot Size: $40,000 USDC

Total HM: 8

Participants: 108

Period: 4 days

Judge: hickuphh3

Total Solo HM: 2

Id: 152

League: ETH

Foundation

Findings Distribution

Researcher Performance

Rank: 82/108

Findings: 1

Award: $40.70

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Foundation

Findings Information

🌟 Selected for report: Dravee

Also found by: 0x040, 0x1f8b, 0xDjango, 0xHarry, 0xNazgul, 0xSmartContract, 0xbepresent, 0xkatana, Amithuddar, Aymen0909, Bnke0x0, Chom, CodingNameKiki, Deivitto, DevABDee, Diraco, ElKu, Fitraldys, Funen, IllIllI, JC, LeoS, Metatron, MiloTruck, Noah3o6, ReyAdmirado, Rohan16, Rolezn, Saw-mon_and_Natalie, Sm4rty, SpaceCake, TomJ, Tomio, Trabajo_de_mates, Waze, Yiko, __141345__, ajtra, apostle0x01, bobirichman, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, d3e4, durianSausage, erictee, fatherOfBlocks, gerdusx, gogo, hakerbaya, ignacio, jag, joestakey, ladboy233, medikko, mics, newfork01, oyc_109, pfapostol, robee, rvierdiiev, sach1r0, saian, samruna, sikorico, simon135, wagmi, zeesaw, zkhorse, zuhaibmohd

Awards

40.6977 USDC - $40.70

Labels

bug

G (Gas Optimization)

old-submission-method

External Links

Link to GitHub issue

[G-01] Redundant zero initialization

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 i; instead of uint256 i = 0;

[G-02] Split up require statements instead of &&

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

One example is

require(
    !callData.startsWith(type(IERC20Approve).interfaceId) &&
    !callData.startsWith(type(IERC20IncreaseAllowance).interfaceId),
    "Split: ERC20 tokens must be split"
);

This can be improved to

require(!callData.startsWith(type(IERC20Approve).interfaceId));
require(!callData.startsWith(type(IERC20IncreaseAllowance).interfaceId),
    "Split: ERC20 tokens must be split"
);

One places had require statements with logical "and"s. Instead, split into two to save gas https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L192

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

[G-03] Cache array length before loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

Locations where this was found include https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L115 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L135 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L227 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L261 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L320

Recommended Mitigation Steps

Cache the array length before the for loop

[G-04] Use != 0 instead of > 0

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Locations where this was found include https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L88 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L131 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/MarketFees.sol#L244 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/mixins/shared/OZERC165Checker.sol#L36

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

[G-05] Short require strings save gas

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

[G-06] Use prefix not postfix in loops

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

[G-07] For loop incrementing can be unsafe

For loops that use i++ do not need to use safemath for this operation because the loop would run out of gas long before this point. Making this addition operation unsafe using unchecked saves gas.

Sample code to make the for loop increment unsafe

for (uint i = 0; i < length; i = unchecked_inc(i)) {
    // do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
    unchecked {
        return i + 1;
    }
}

Idea borrowed from https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#the-increment-in-for-loop-post-condition-can-be-made-unchecked

Locations where this was found include https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L227 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L261 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/FETH.sol#L591 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/FETH.sol#L718 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/FETH.sol#L760 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/FETH.sol#L780 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/FETH.sol#L807

Recommended Mitigation Steps

Make the increment in for loops unsafe to save gas

[G-08] Use iszero assembly for zero checks

Comparing a value to zero can be done using the iszero EVM opcode. This can save gas

Source from t11s https://twitter.com/transmissions11/status/1474465495243898885

Recommended Mitigation Steps

Use the assembly iszero evm opcode to compare values to zero

[G-09] Add payable to functions that won't receive ETH

Identifying a function as payable saves gas. Functions that have a modifier like onlyOwner or auth cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

There are many functions that have the auth modifier in the contracts. Some examples are https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L202 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTCollectionFactory.sol#L226 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L159 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L195 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L209 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/NFTDropCollection.sol#L220

Recommended Mitigation Steps

Add payable to these functions for gas savings

[G-10] Add payable to constructors that won't receive ETH

Identifying a constructor as payable saves gas. Constructors should only be called by the admin or deployer and should not mistakenly receive ETH. Constructors can be payable to save gas.

Recommended Mitigation Steps

Add payable to these functions for gas savings

[G-11] Use internal function in place of modifier

An internal function can save gas vs. a modifier. A modifier inlines the code of the original function but an internal function does not.

Source https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#dde7

Recommended Mitigation Steps

Use internal functions in place of modifiers to save gas.

[G-12] Use type(uint128).max

Using type(uint128).max uses less gas than other options for the constant value uint256 private constant last128BitsMask = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;. Source https://forum.openzeppelin.com/t/using-the-maximum-integer-in-solidity/3000/13

Locations where this was found include https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/LockedBalance.sol#L24

Recommended Mitigation Steps

Use type(uint128).max

[G-13] Use uint not bool

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.

Recommended Mitigation Steps

Replace bool variables with uints

[G-14] Use uint256 not smaller ints

From the solidity docs

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.

Recommended Mitigation Steps

Replace smaller int or uint variables with uint256 variables

[G-15] Use newer solidity version

A solidity version before 0.8.X is used in this project. The latest release of solidity includes changes that can provide gas savings. The improvements include: The advantages of versions 0.8.* over <0.8.0 are:

Solidity version 0.8.13 can save more gas with Yul IR pipeline

Recommended Mitigation Steps

Use solidity release 0.8.13 or above with Yul IR pipeline and other improvements for gas savings

[G-16] Use Solidity errors instead of require

Solidity errors introduced in version 0.8.4 can save gas on revert conditions https://blog.soliditylang.org/2021/04/21/custom-errors/ https://twitter.com/PatrickAlphaC/status/1505197417884528640

Recommended Mitigation Steps

Replace require blocks with new solidity errors described in https://blog.soliditylang.org/2021/04/21/custom-errors/

[G-17] Bitshift for divide by 2

When multiply or dividing by a power of two, it is cheaper to bitshift than to use standard math operations.

There is a divide by 2 operation on these lines https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/FETH.sol#L210 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/libraries/LockedBalance.sol#L100

Recommended Mitigation Steps

Bitshift right by one bit instead of dividing by 2 to save gas

[G-18] Non-public variables save gas

Many constant variables are public, but changing the visibility of these variables to private or internal can save gas.

Recommended Mitigation Steps

Declare some public variables as private or internal to save gas

[G-19] Use calldata instead of memory for function arguments

Using calldata instead of memory for function arguments saves gas sometimes. This can happen when a function is called externally and the memory array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). If the array is used in a for loop, arr[i] accesses the value in memory using a mload. If calldata is used instead, then instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Many cases of function arguments using memory instead of calldata can use this improvement to save gas https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L129 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L167 https://github.com/code-423n4/2022-08-foundation/tree/main/contracts/PercentSplitETH.sol#L292

Source https://gist.github.com/hrkrshnn/ee8fabd532058307229d65dcd5836ddc#use-calldata-instead-of-memory-for-function-parameters

Recommended Mitigation Steps

Change function arguments from memory to calldata

[G-20] Write contracts in vyper

The contracts are all written entirely in solidity. Writing contracts with vyper instead of solidity can save gas.

Source https://twitter.com/eiber_david/status/1515737811881807876 doggo demonstrates https://twitter.com/fubuloubu/status/1528179581974417414?t=-hcq_26JFDaHdAQZ-wYxCA&s=19

Recommended Mitigation Steps

Write some or all of the contracts in vyper to save gas

#0 - HardlyDifficult
2022-08-19T00:07:18Z

Don't initialize variables with default values.

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

[G-02] Split up require statements instead

This may save some gas, but for readability I prefer the current form.

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.

++i costs less than i++

Agree and will fix.

[G-07] For loop incrementing can be unsafe

Invalid -- all examples provided are already unchecked.

Use assembly to check for zero address.

Won't fix. While this may save a tiny bit of gas, we reserve the use of assembly to areas where it would provide significant benefit or accomplish something that's otherwise not possible with Solidity alone.

Functions guaranteed to revert when called by normal users can be marked payable

Disagree. This could save gas but 1) this seems like a poor practice to encourage and could be error prone. and 2) we don't care about optimizing admin-only actions.

[G-11] Use internal function in place of modifier

Disagree, this can increase user costs which is more important than deployment costs.

[G-12] Use type(uint128).max

This may save a bit on deployment, but for consistency and readability in that context I prefer the current form.

[G-13] Use uint not bool

Agree and we are considering a change.

uints/ints 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.

[G-15] Use newer solidity version

Invalid, we are compiling with the latest version 0.8.16 per the hardhat.config.

Custom errors

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.

[G-17] Bitshift for divide by 2

The first example is not a / 2 -- invalid. The second was out of scope for this contest, but we'll consider a change there.

Using private rather than public for constants to saves gas.

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

calldata

Valid and will fix, but the examples listed were out of scope for this contest.

[G-20] Write contracts in vyper

lol thanks

#1 - HickupHH3
2022-09-05T06:27:22Z

[G-20] Write contracts in vyper

lolol

Foundation Drop contest - 0xkatana's results

Foundation is a web3 destination.

General Information

Findings Distribution

Researcher Performance

External Links

[G-07] Gas Report - $40.70

Findings Information

Awards

Labels

External Links

[G-01] Redundant zero initialization

Recommended Mitigation Steps

[G-02] Split up require statements instead of &&

Recommended Mitigation Steps

[G-03] Cache array length before loop

Recommended Mitigation Steps

[G-04] Use != 0 instead of > 0

Recommended Mitigation Steps

[G-05] Short require strings save gas

Recommended Mitigation Steps

[G-06] Use prefix not postfix in loops

Recommended Mitigation Steps

[G-07] For loop incrementing can be unsafe

Recommended Mitigation Steps

[G-08] Use iszero assembly for zero checks

Recommended Mitigation Steps

[G-09] Add payable to functions that won't receive ETH

Recommended Mitigation Steps

[G-10] Add payable to constructors that won't receive ETH

Recommended Mitigation Steps

[G-11] Use internal function in place of modifier

Recommended Mitigation Steps

[G-12] Use type(uint128).max

Recommended Mitigation Steps

[G-13] Use uint not bool

Recommended Mitigation Steps

[G-14] Use uint256 not smaller ints

Recommended Mitigation Steps

[G-15] Use newer solidity version

Recommended Mitigation Steps

[G-16] Use Solidity errors instead of require

Recommended Mitigation Steps

[G-17] Bitshift for divide by 2

Recommended Mitigation Steps

[G-18] Non-public variables save gas

Recommended Mitigation Steps

[G-19] Use calldata instead of memory for function arguments

Recommended Mitigation Steps

[G-20] Write contracts in vyper

Recommended Mitigation Steps

#0 - HardlyDifficult2022-08-19T00:07:18Z

#1 - HickupHH32022-09-05T06:27:22Z

#0 - HardlyDifficult
2022-08-19T00:07:18Z

#1 - HickupHH3
2022-09-05T06:27:22Z