Forgeries contest - IllIllI's results

Summary

Low Risk Issues

Total: 2 instances over 1 issues

Non-critical Issues

Total: 16 instances over 9 issues

Low Risk Issues

[L‑01] Use Ownable2Step rather than Ownable

Ownable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.

There are 2 instances of this issue:

File: src/VRFNFTRandomDrawFactory.sol

14    contract VRFNFTRandomDrawFactory is
15        IVRFNFTRandomDrawFactory,
16        OwnableUpgradeable,
17        UUPSUpgradeable,
18:       Version(1)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L14-L18

File: src/VRFNFTRandomDraw.sol

15    contract VRFNFTRandomDraw is
16        IVRFNFTRandomDraw,
17        VRFConsumerBaseV2,
18        OwnableUpgradeable,
19:       Version(1)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L15-L19

Non-critical Issues

[N‑01] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

There are 2 instances of this issue:

File: src/VRFNFTRandomDrawFactory.sol

14    contract VRFNFTRandomDrawFactory is
15        IVRFNFTRandomDrawFactory,
16        OwnableUpgradeable,
17        UUPSUpgradeable,
18:       Version(1)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L14-L18

File: src/VRFNFTRandomDraw.sol

15    contract VRFNFTRandomDraw is
16        IVRFNFTRandomDraw,
17        VRFConsumerBaseV2,
18        OwnableUpgradeable,
19:       Version(1)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L15-L19

[N‑02] override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

There is 1 instance of this issue:

File: src/VRFNFTRandomDrawFactory.sol

55:       function _authorizeUpgrade(address newImplementation)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L55

[N‑03] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There is 1 instance of this issue:

File: src/VRFNFTRandomDraw.sol

/// @audit 12
95:               block.timestamp + (MONTH_IN_SECONDS * 12)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L95

[N‑04] Numeric values having to do with time should use time units for readability

There are units for seconds, minutes, hours, days, and weeks, and since they're defined, they should be used

There are 4 instances of this issue:

File: src/VRFNFTRandomDraw.sol

/// @audit 60
/// @audit 60
29:       uint256 immutable HOUR_IN_SECONDS = 60 * 60;

/// @audit 3600
31:       uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7);

/// @audit 3600
33:       uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L29

[N‑05] Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

There is 1 instance of this issue:

File: src/interfaces/IVRFNFTRandomDraw.sol

64:           /// @notice has chosen a random number (in case random number = 0(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0)(in case random number = 0))

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDraw.sol#L64

[N‑06] Non-library/interface files should use fixed compiler versions, not floating ones

There is 1 instance of this issue:

File: src/utils/Version.sol

2:    pragma solidity ^0.8.10;

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/utils/Version.sol#L2

[N‑07] NatSpec is incomplete

There are 4 instances of this issue:

File: src/interfaces/IVRFNFTRandomDrawFactory.sol

/// @audit Missing: '@return'
20        /// @notice Function to make a new drawing
21        /// @param settings settings for the new drawing
22        function makeNewDraw(IVRFNFTRandomDraw.Settings memory settings)
23            external
24:           returns (address);

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDrawFactory.sol#L20-L24

File: src/interfaces/IVRFNFTRandomDraw.sol

/// @audit Missing: '@return'
106       /// @notice Function to determine if the user has won in the current drawing
107       /// @param user address for the user to check if they have won in the current drawing
108:      function hasUserWon(address user) external view returns (bool);

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDraw.sol#L106-L108

File: src/VRFNFTRandomDrawFactory.sol

/// @audit Missing: '@return'
36        /// @notice Function to make a new drawing
37        /// @param settings settings for the new drawing
38        function makeNewDraw(IVRFNFTRandomDraw.Settings memory settings)
39            external
40:           returns (address)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L36-L40

File: src/VRFNFTRandomDraw.sol

/// @audit Missing: '@return'
262       /// @notice Function to determine if the user has won in the current drawing
263       /// @param user address for the user to check if they have won in the current drawing
264:      function hasUserWon(address user) public view returns (bool) {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L262-L264

[N‑08] Contracts should have full test coverage

While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.

There is 1 instance of this issue:

File: src/interfaces/IVRFNFTRandomDrawFactory.sol

[N‑09] Large or complicated code bases should implement fuzzing tests

Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.

There is 1 instance of this issue:

File: src/interfaces/IVRFNFTRandomDrawFactory.sol

---

Excluded findings

These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness

Summary

Low Risk Issues

Total: 3 instances over 1 issues

Non-critical Issues

Total: 5 instances over 2 issues

Low Risk Issues

[L‑01] Missing checks for address(0x0) when assigning values to address state variables

There are 3 instances of this issue:

File: src/ownable/OwnableUpgradeable.sol

/// @audit (valid but excluded finding)
62:           _owner = _initialOwner;

/// @audit (valid but excluded finding)
93:           _owner = _newOwner;

/// @audit (valid but excluded finding)
107:          _pendingOwner = _newOwner;

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/OwnableUpgradeable.sol#L62

Non-critical Issues

[N‑01] public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There is 1 instance of this issue:

File: src/VRFNFTRandomDraw.sol

/// @audit (valid but excluded finding)
75        function initialize(address admin, Settings memory _settings)
76            public
77:           initializer

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L75-L77

[N‑02] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

There are 4 instances of this issue:

File: src/interfaces/IVRFNFTRandomDrawFactory.sol

/// @audit (valid but excluded finding)
11:       event SetupNewDrawing(address user, address drawing);

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDrawFactory.sol#L11

File: src/interfaces/IVRFNFTRandomDraw.sol

/// @audit (valid but excluded finding)
43:       event InitializedDraw(address indexed sender, Settings settings);

/// @audit (valid but excluded finding)
45:       event SetupDraw(address indexed sender, Settings settings);

/// @audit (valid but excluded finding)
49:       event DiceRollComplete(address indexed sender, CurrentRequest request);

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDraw.sol#L43

Summary

Gas Optimizations

Total: 21 instances over 4 issues with 655 gas saved

Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.

Gas Optimizations

[G‑01] Structs can be packed into fewer storage slots

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings

There is 1 instance of this issue:

File: src/interfaces/IVRFNFTRandomDraw.sol

/// @audit Variable ordering with 8 slots instead of the current 9:
///           uint256(32):tokenId, uint256(32):drawingTokenStartId, uint256(32):drawingTokenEndId, uint256(32):drawBufferTime, uint256(32):recoverTimelock, bytes32(32):keyHash, address(20):token, uint64(8):subscriptionId, address(20):drawingToken
71        struct Settings {
72            /// @notice Token Contract to put up for raffle
73            address token;
74            /// @notice Token ID to put up for raffle
75            uint256 tokenId;
76            /// @notice Token that each (sequential) ID has a entry in the raffle.
77            address drawingToken;
78            /// @notice Start token ID for the drawing (if totalSupply = 20 but the first token is 5 (5-25), setting this to 5 would fix the ordering)
79            uint256 drawingTokenStartId;
80            /// @notice End token ID for the drawing (exclusive) (token ids 0 - 9 would be 10 in this field)
81            uint256 drawingTokenEndId;
82            /// @notice Draw buffer time – time until a re-drawing can occur if the selected user cannot or does not claim the NFT.
83            uint256 drawBufferTime;
84            /// @notice block.timestamp that the admin can recover the NFT (as a safety fallback)
85            uint256 recoverTimelock;
86            /// @notice Chainlink gas keyhash
87            bytes32 keyHash;
88            /// @notice Chainlink subscription id
89            uint64 subscriptionId;
90:       }

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDraw.sol#L71-L90

[G‑02] State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There are 3 instances of this issue:

File: src/ownable/OwnableUpgradeable.sol

/// @audit _pendingOwner on line 95
96:               delete _pendingOwner;

/// @audit _pendingOwner on line 122
124:          delete _pendingOwner;

/// @audit _pendingOwner on line 129
131:          delete _pendingOwner;

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/OwnableUpgradeable.sol#L96

[G‑03] Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There are 7 instances of this issue:

File: src/interfaces/IVRFNFTRandomDrawFactory.sol

/// @audit initialize(), makeNewDraw()
6:    interface IVRFNFTRandomDrawFactory {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDrawFactory.sol#L6

File: src/interfaces/IVRFNFTRandomDraw.sol

/// @audit initialize(), startDraw(), redraw(), hasUserWon(), winnerClaimNFT(), lastResortTimelockOwnerClaimNFT(), getRequestDetails()
4:    interface IVRFNFTRandomDraw {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/interfaces/IVRFNFTRandomDraw.sol#L4

File: src/ownable/IOwnableUpgradeable.sol

/// @audit pendingOwner(), safeTransferOwnership(), cancelOwnershipTransfer()
7:    interface IOwnableUpgradeable {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/IOwnableUpgradeable.sol#L7

File: src/ownable/OwnableUpgradeable.sol

/// @audit pendingOwner(), safeTransferOwnership(), resignOwnership(), cancelOwnershipTransfer()
12:   abstract contract OwnableUpgradeable is IOwnableUpgradeable, Initializable {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/OwnableUpgradeable.sol#L12

File: src/utils/Version.sol

/// @audit contractVersion()
4:    contract Version {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/utils/Version.sol#L4

File: src/VRFNFTRandomDrawFactory.sol

/// @audit initialize(), makeNewDraw()
14:   contract VRFNFTRandomDrawFactory is

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L14

File: src/VRFNFTRandomDraw.sol

/// @audit getRequestDetails(), initialize(), startDraw(), redraw(), hasUserWon(), winnerClaimNFT(), lastResortTimelockOwnerClaimNFT()
15:   contract VRFNFTRandomDraw is

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L15

[G‑04] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 10 instances of this issue:

File: src/ownable/OwnableUpgradeable.sol

57        function __Ownable_init(address _initialOwner)
58            internal
59            notZeroAddress(_initialOwner)
60:           onlyInitializing

79        function transferOwnership(address _newOwner)
80            public
81            notZeroAddress(_newOwner)
82:           onlyOwner

102       function safeTransferOwnership(address _newOwner)
103           public
104           notZeroAddress(_newOwner)
105:          onlyOwner

114:      function resignOwnership() public onlyOwner {

119:      function acceptOwnership() public onlyPendingOwner {

128:      function cancelOwnershipTransfer() public onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/ownable/OwnableUpgradeable.sol#L57-L60

File: src/VRFNFTRandomDrawFactory.sol

55        function _authorizeUpgrade(address newImplementation)
56            internal
57            override
58:           onlyOwner

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L55-L58

File: src/VRFNFTRandomDraw.sol

173:      function startDraw() external onlyOwner returns (uint256) {

203:      function redraw() external onlyOwner returns (uint256) {

304:      function lastResortTimelockOwnerClaimNFT() external onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L173

---

Excluded findings

These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness

Summary

Gas Optimizations

Total: 2 instances over 1 issues with 240 gas saved

Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.

Gas Optimizations

[G‑01] Using calldata instead of memory for read-only arguments in external functions saves gas

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. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.

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's 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

Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

There are 2 instances of this issue:

File: src/VRFNFTRandomDrawFactory.sol

/// @audit settings - (valid but excluded finding)
38        function makeNewDraw(IVRFNFTRandomDraw.Settings memory settings)
39            external
40:           returns (address)

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L38-L40

File: src/VRFNFTRandomDraw.sol

/// @audit _settings - (valid but excluded finding)
75        function initialize(address admin, Settings memory _settings)
76            public
77:           initializer

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L75-L77