Platform: Code4rena
Start Date: 13/12/2022
Pot Size: $36,500 USDC
Total HM: 5
Participants: 77
Period: 3 days
Judge: gzeon
Total Solo HM: 1
Id: 191
League: ETH
Rank: 34/77
Findings: 2
Award: $71.66
🌟 Selected for report: 0
🚀 Solo Findings: 0
45.7078 USDC - $45.71
The codebase is secure and well-structured. A few minor code-style issues and execution ordering errors are listed below.
redraw() can be called before startDraw()VRFNFTRandomDraw.redraw() can be successfully called before the first draw has been started through startDraw() by transferrig the NFT to the contract and then calling redraw(). This does not cause any substantial risk to the NFT, but it causes the contract to not emit the SetupDraw event when the first draw starts, potentially causing off-chain software tracking the events to have issues.
redraw() does not emit an eventVRFNFTRandomDraw.redraw() does not emit an event, e.g. Redraw, making it hard for off-chain software to monitor for redraws.
constant over immutableconstant values are known at compile time and unchangeable. immutable values can be changed in the constructor and are unchangeable thereafter. If an immutable value is not changed in the constructor, the constant keyword should be used.
| File | Lines | Description |
|---|---|---|
src/VRFNFTRandomDraw.sol | 22 | callbackGasLimit |
src/VRFNFTRandomDraw.sol | 24 | minimumRequestConfirmations |
src/VRFNFTRandomDraw.sol | 26 | wordsRequested |
src/VRFNFTRandomDraw.sol | 29 | HOUR_IN_SECONDS |
src/VRFNFTRandomDraw.sol | 31 | WEEK_IN_SECONDS |
src/VRFNFTRandomDraw.sol | 33 | MONTH_IN_SECONDS |
Visibility for state variables should be explicitly specified, even if the default internal is intended.
| File | Lines | Description |
|---|---|---|
src/VRFNFTRandomDraw.sol | 22 | callbackGasLimit |
src/VRFNFTRandomDraw.sol | 24 | minimumRequestConfirmations |
src/VRFNFTRandomDraw.sol | 26 | wordsRequested |
src/VRFNFTRandomDraw.sol | 29 | HOUR_IN_SECONDS |
src/VRFNFTRandomDraw.sol | 31 | WEEK_IN_SECONDS |
src/VRFNFTRandomDraw.sol | 33 | MONTH_IN_SECONDS |
src/VRFNFTRandomDraw.sol | 37 | coordinator |
#0 - c4-judge
2022-12-17T17:22:58Z
gzeon-c4 marked the issue as grade-b
🌟 Selected for report: c3phas
Also found by: 0x1f8b, Aymen0909, Bnke0x0, Bobface, IllIllI, PaludoX0, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Sathish9098, adriro, chaduke, codeislight, ctrlc03, indijanc, izhelyazkov, kuldeep, nadin, neko_nyaa, nicobevi, rvierdiiev, shark
25.9485 USDC - $25.95
A constant value is dynamically computed. It should be stored as constant to avoid recomputation.
| File | Lines | Description |
|---|---|---|
src/VRFNFTRandomDraw.sol | 95 | (MONTH_IN_SECONDS * 12) can be stored as constant YEAR_IN_SECONDS |
A condition is unnecessarily checked multiple time. The duplicate check should be removed.
| File | Lines | Description |
|---|---|---|
src/VRFNFTRandomDraw.sol | 126 - 137 | initialize checks that the owner of the contract owns the NFT to be raffled. However, it does not transfer in the NFT. The NFT is only transferred in in startDraw. The check in initialize can be removed, since if the NFT is not owned or does not exist, the check in startDraw will catch this condition. |
src/VRFNFTRandomDraw.sol | 175 - 177 | Checking for request.currentChainlinkRequestId != 0 is done again in _requestRoll(). |
src/utils/OwnableUpgradeable.sol | 95 | The statement first reads (SLOAD) the storage slots, checks if for zero, and if it is non-zero, deletes (SSTORE) it. Removing the if guard and just always executing delete _pendingOwner is cheaper, since the overhead through the if condition is removed. |
msg.sender in a local variableThe EVM offers the CALLER opcode which only costs 2 gas (see here). msg.sender is directly translated to the CALLER opcode, which makes it more expensive to store msg.sender in a local stack variable due to the required stack operations. Using msg.sender directly where it is requied is cheaper.
| File | Lines | Description |
|---|---|---|
src/VRFNFTRandomDraw.sol | 279 | address user = msg.sender; |
src/VRFNFTRandomDrawFactory.sol | 42 | address admin = msg.sender; |
Using smaller types where possible in structs which are stored in storage decreases the type's size and thus requires less storage slots, making it cheaper to read and write the struct from and to storage.
| File | Lines | Description |
|---|---|---|
src/interfaces/IVRNFTRandomDraw.sol | 67 | drawTimelock can be uint128, saving one storage slot |
src/interfaces/IVRNFTRandomDraw.sol | 83, 85 | drawBufferTime and recoverTimelock can be uint128, saving one storage slot |
#0 - c4-judge
2022-12-17T17:44:30Z
gzeon-c4 marked the issue as grade-b