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: 56/108
Findings: 2
Award: $61.81
🌟 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
41.2059 USDC - $41.21
The following calls ignores the return value of the called function that might indicate the the call failed.
Some contracts does not support 0 transfer, then the transaction will revert with no explanation. We recommend to add a require statement that the amount is not 0.
There is no check for the access to be in the array bounds.
The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.
Missing checks for zero-addresses may lead to infunctional protocol. In this case the function is an initializer then the value can be passed only once and is important to be validated. If the variable addresses are updated incorrectly.
In case a hack is occuring or an exploit is discovered, the team (or validators in this case) should be able to pause functionality until the necessary changes are made to the system. Additionally, the gravity.sol contract should be manged by proxy so that upgrades can be made by the validators.
Because an attack would probably span a number of blocks, a method for pausing the contract would be able to interrupt any such attack if discovered.)
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
I didn't see a use of using payable in the following functions, consider changing it.
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. (SWC-103)
Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.
To record the initialize parameters for off-chain monitoring and transparency reasons, you might find it useful to emit an event after the initialize() functions
The emitted event is not indexed, making off-chain scripts such as front-ends of dApps difficult to filter the events efficiently.
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
#0 - HardlyDifficult
2022-08-18T20:21:41Z
Unused success return value
Invalid: the first two fall into https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/PercentSplitETH.sol#L250 And the 3rd link does not have a return value.
Missing 0 address check at transfer
Thanks for this feedback but those contracts were out of scope for this contest.
Array access is out of bounds
Thanks for this feedback but those contracts were out of scope for this contest.
Different solidity versions are in use
Invalid - all contracts are compiled with 0.8.16.
Missing zero address check for initializers functions
The collections inherit guarantees for this from the factory. Agree with Treasury but that was out of scope.
Contract should have pause/unpause functionality
This is a fair suggestion but we do not intend to add a pause feature at this time.
Should approve(0) first
Invalid - we do not have general ERC20 support.
Make sure the following functions has to be payable
Invalid - payable is required for these.
Avoid floating pragma
Disagree. We intentionally use a floating pragma in order to make integrating with contracts easier. Other contract developers are looking to interact with our contracts and they may be on a different version than we use. The pragma selected for our contracts is the minimum required in order to correctly compile and function. This way integration is easier if they lag a few versions behind, or if they use the latest but we don't bump our packages frequently enough, and when we do upgrade versions unless there was a breaking solidity change -- it should just swap in by incrementing our npm package version.
Some of the following function specification is missing
Invalid - for the in scope contracts, full natspec is already included there.
Consider removing the unused parameters names in the following functions
Invalid - the empty blocks linked are required to avoid compile errors.
Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons.
Agree that we were inconsistent with these checks. We have moved the NFTDropCollection requirement into the factory so that both collection types are implemented in a similar fashion, and we went with the factory instead of the collection init in order to follow the best practice of fail fast.
Missing an event after critical initialize() functions
Fair feedback, but I don't believe events are required or warranted in those examples.
Not indexed events
I believe this is invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.
Events not emitted for important state changes
FETHNode.sol#L22 I don't believe an event is required in the constructor here.
SequentialMintCollection.sol#L62 This information is already emitted by the factory which called this.
NFTCollectionFactory.sol#L192 This information is available via an event when the first template is assigned.
NFTDropCollection.sol#L266 Invalid: this is a read only function.
FETH.sol#L200 Out of scope, and I don't believe an event is required for the constructor.
Add event to the following functions
This seems redundant with suggestions above.
🌟 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.6008 USDC - $20.60
reading msg.sender is 2 gas units which is less than a read of a local var + the unnecessary store operation.
In the following require statements you can use custom errors to save gas and improve code quality.
#0 - HardlyDifficult
2022-08-17T18:57:39Z
Don't cache msg.sender
Invalid. None of those links are using msg.sender.. (?)
Using abiEncodePacked() is more efficient that abiEncode()
Invalid... while it seems like this should help, changing to .encode makes creating collections slightly more expensive. createNFTCollection increased by 9 gas and createNFTDropCollection increased by 8-10 gas
Use 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.
If the function is onlyOwner you may make it payable to reduce gas usage.
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
Use assembly opcodes iszero instead of solidity equation to save gas
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.