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
Rank: 84/108
Findings: 2
Award: $33.77
馃専 Selected for report: 0
馃殌 Solo Findings: 0
馃専 Selected for report: rbserver
Also found by: 0xc0ffEE, CodingNameKiki, Deivitto, Diraco, IllIllI, KIntern_NA, Lambda, Noah3o6, Treasure-Seeker, ignacio, oyc_109, zeesaw
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L143 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L159 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L271
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#L449
#0 - HardlyDifficult
2022-08-18T21:30:59Z
Use safeMint
Agree will fix - for context see our response here.
abi.encodePacked() should not be used
It's a gas savings to use encodePacked for the use cases we support here. There does not appear to be a compelling reason to change.
#1 - HickupHH3
2022-08-31T11:06:24Z
dup of #183
馃専 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
20.6 USDC - $20.60
The overheads outlined below are PER LOOP, excluding the first loop storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset ( ++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- AND WHILE-LOOPS ++I COSTS LESS GAS THAN I++ https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#L126 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#L198 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#L484 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#L503
#3 Check msg.value first - 1 gas per instance - 3 gas total Reading msg.value costs 2 gas, while reading from memory costs 3, this will save 1 gas with no downside https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/FETHNode.sol#L56
!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#L244
When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it鈥檚 still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#L371 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L105 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L282
#0 - HardlyDifficult
2022-08-17T09:03:25Z
1).LENGTH SHOULD NOT BE LOOKED UP IN EVERY LOOP OF A FOR-LOOP and Increments can be unchecked
4 instances were linked and 3 of those are already unchecked -- so those are invalid. The other is a read only function not intended to be used on-chain, but as a best practice we will add unchecked there as well.
2 += COSTS MORE GAS THAN = + FOR STATE VARIABLES
No impact. We made the recommended change and ran gas-reporter for our entire test suite. 0 impact detected. Even if there were a small benefit, we feel this hurts readability and would not be worth trivial savings.
3 Check msg.value first - 1 gas per instance - 3 gas total
Invalid. For the market in scope for this contest, shouldRefundSurplus is always false -- so the recommendation would prevent fail fast and cause gas to regress:
Running test/NFTDropMarket/fixedPrice/* Before: - 112650 路 663751 路 278013 After: - 112655 路 663756 路 278016 Impact: +5 gas
4 Use != 0 instead of > 0 at the above mentioned codes. The variable is uint, so it will not be below 0 so it can just check != 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
5 USING CALLDATA INSTEAD OF MEMORY FOR READ-ONLY ARGUMENTS IN EXTERNAL FUNCTIONS SAVES GAS
3 examples were included:
createNFTDropCollectionWithPaymentFactory and initialize valid & will fix. This saves ~50 gas on createNFTCollection and ~60 gas on createNFTDropCollectionWithPaymentFactorybaseURI invalid, the memory reference here is a return param - calldata cannot be used.