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: 47/108
Findings: 2
Award: $62.96
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Saw-mon_and_Natalie
Also found by: 0x1f8b, 0x52, 0xDjango, 0xNazgul, 0xSmartContract, 0xSolus, 0xackermann, 0xmatt, 0xsolstars, Aymen0909, Bnke0x0, Chom, Deivitto, DevABDee, Dravee, ElKu, IllIllI, JC, Kumpa, Lambda, LeoS, MiloTruck, PwnedNoMore, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Treasure-Seeker, Vexjon, Waze, Yiko, __141345__, apostle0x01, auditor0517, berndartmueller, bin2chen, bobirichman, brgltd, bulej93, c3phas, carlitox477, cccz, cryptphi, csanuragjain, d3e4, danb, delfin454000, durianSausage, erictee, fatherOfBlocks, gogo, iamwhitelights, joestakey, jonatascm, ladboy233, mics, oyc_109, rbserver, ret2basic, robee, rokinot, rvierdiiev, shenwilly, sikorico, simon135, thank_you, wagmi, yash90, zeesaw, zkhorse
42.2838 USDC - $42.28
L206, L230 The comment // Version cannot overflow 256 bits. is misleading as referenced variable is uint32.
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L206
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L230
Inconsistent use of BASIS_POINTS and hardcoding.
CREATOR_ROYALTY_DENOMINATOR is calculated (L45) using both the constant BASIS_POINTS but its final value is determined by hardcoding. It is used to divide an actual uint value and as such is independent from BASIS_POINTS which represents a whole. Usage here of BASIS_POINTS is at best redundant and misleading, and at worst an imprudent change of its value would cause an unintended change in CREATOR_ROYALTY_DENOMINATOR. Replace L45 with uint256 private constant CREATOR_ROYALTY_DENOMINATOR = 10; // 10%, and similarly for L47.
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L45-L47
Potential for reentrancy.
If _tryUseFETHBalance (FETHNode.sol L46-L63) is called such that the else-if-condition at L56 passes then msg.sender can reenter at L60. This is not currently an issue as it is never called in this way, but if, for example, in NFTDropMarketFixedPriceSale.sol the function mintFromFixedPriceSale (L170-L219) had allowed a surplus payment (i.e. msg.value > mintCost does not revert at L201) with the intention to then refund it by replacing L204 with _tryUseFETHBalance(mintCost, true);, which would seem reasonable, then an attacker could reenter to buy more NFTs before they are minted to him and his balance is updated, such that he can exceed the saleConfig.limitPerAccount checked for at L183.
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/FETHNode.sol#L46-L63
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L170-L219
#0 - HardlyDifficult
2022-08-18T16:02:46Z
// Version cannot overflow 256 bits.
Agree, this was missed when we started packing. Will fix.
Inconsistent _DENOMINATOR
Disagree, although this is just code style. We went with the current approach in order to simplify the implementation below.
Potential for reentrancy in _tryUseFETHBalance
This is potentially valid, would need to create a POC to confirm. Based on other feedback, we have changed the implementation to confirm the balance after minting completes which will address the potential reentrancy concern here as well.
#1 - c4-sponsor
2022-08-30T13:48:40Z
Added on behalf of @Simon-Busch
#2 - c4-sponsor
2022-08-30T13:48:56Z
test || Action reverted on behalf of @Simon-Busch
#3 - c4-sponsor
2022-08-30T13:49:04Z
Added on behalf of @Simon-Busch
🌟 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.6764 USDC - $20.68
Preincrementing may be cheaper than postincrementing:
L207: Change versionNFTCollection++ to ++versionNFTCollection.
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L207
L231: Change versionNFTDropCollection++ to ++versionNFTDropCollection.
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L231
L198-L200 Can be unchecked{} with same reasoning as provided in L126-L135.
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L198-L200
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L126-L135
Precalculate <>.length before for-loop. Instances: L126, L198, L484, L503.
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L126
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L198
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L484
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L503
L445 Consider evaluating the cheaper condition first in (creatorRecipients.length != 0 || assumePrimarySale)
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L445
#0 - HardlyDifficult
2022-08-19T15:32:14Z
++i costs less than i++
Agree and will fix.
unchecked loop in
getFeesAndRecipients
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.
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.
L445 Consider evaluating the cheaper condition first
Fair point, will consider this.