Forgeries contest - shark's results

1. Unused imports

Contracts that are imported and not used anywhere in the code are most likely an accident due to incomplete refactoring. Such imports can lead to confusion among readers.

Here is an example of this issue:

File: VRFNFTRandomDraw.sol#L8

import {VRFCoordinatorV2, VRFCoordinatorV2Interface} from "@chainlink/contracts/src/v0.8/VRFCoordinatorV2.sol";

In the example above, VRFCoordinatorV2 is never used in the contract. As such it can be removed from the import like so:

import {VRFCoordinatorV2Interface} from "@chainlink/contracts/src/v0.8/VRFCoordinatorV2.sol";

2. Empty functions

Empty functions can reduce readability because readers need to guess whether it’s intentional or not. So writing a clear comment for empty functions is a good practice. Additionally, an empty function should also revert on call/emit an event.

• File: VRFNFTRandomDrawFactory.sol Line 55-59

3. Typos

File: OwnableUpgradeable.sol Line 113

    /// @audit "callably" -> "callable"
    /// @dev only callably by the owner, dangerous call.

File: IVRFNFTRandomDraw.sol Line 46

    /// @audit "aftr" -> "after"
    /// @notice When the owner reclaims nft aftr recovery time delay

4. Empty Event

The event below has no parameters in it to emit anything. Consider either removing this event or refactoring it with relevant info.

File: IVRFNFTRandomDrawFactory.sol Line 18

    event SetupFactory();

5. Limit line length

To comply with the Solidity Style Guide, lines should be limited to 120 characters max.

For example, the instance listed below is way over the limit of 120 characters:

File: IVRFNFTRandomDraw.sol Line 64

6. Unspecific Pragma Version

Locking the pragma helps ensure that contracts don't get deployed with unintended versions, for example, the latest compiler version which could have higher risks of undiscovered bugs.

For example, The pragma version in Version.sol is unspecific (pragma solidity ^0.8.10;).

7. No storage gap for upgradeable contracts

For upgradeable contracts, there should be a storage gap at the end of the contract. Without a storage gap, Storage clashes could occur while using inheritance.

For example:

File: VRFNFTRandomDrawFactory.sol

Consider adding the variable __gap at the end of VRFNFTRandomDrawFactory.sol:

uint256[50] private __gap;

1. Functions with access control marked as payable cost less gas

Functions with access control will cost less gas when marked as payable (assuming the caller has correct permissions). This is because the compiler doesn't need to check for msg.value, saving approximately 20 gas per call.

Here is an example of this issue:

File: VRFNFTRandomDraw.sol Line 203

    function redraw() external onlyOwner returns (uint256) {

As seen above, the function has access control (onlyOwner).

As such, the function could be marked payable:

    function redraw() external payable onlyOwner returns (uint256) {

Here are a few other instances:

• File: VRFNFTRandomDraw.sol Line 173
• File: VRFNFTRandomDraw.sol Line 304

2. Caching state variables saves gas

If a state variable is re-read multiple times, consider caching the state variable in a local variable.

For example:

File: VRFNFTRandomDraw.sol Line 230-274

In the function fulfillRandomWords, the state variable settings is re-read at Line 249, Line 250, and Line 256.

To optimize gas savings, consider caching settings in a local variable:

Settings memory _settings = settings

3. Unnecessary msg.sender cache

Caching msg.sender doesn't save gas. In fact, reading from msg.sender will cost approximately 2 less gas compared to reading from a local variable. Also, caching msg.sender is an unnecessary store operation.

There are 2 instances of this issue:

• File: VRFNFTRandomDrawFactory.sol Line 42
• File: VRFNFTRandomDraw.sol Line 279

4. State variable is being read instead of memory variable

In VRFNFTRandomDraw.sol, the following emit InitializedDraw() is using settings (state variable) instead of _settings (memory variable). As such, reading from a state variable will cost more gas.

File: VRFNFTRandomDraw.sol Line 123

        emit InitializedDraw(msg.sender, settings);

As you can see, the emitted event is using settings instead of _settings.

As such, the code could be refactored to use _settings instead of settings:

        emit InitializedDraw(msg.sender, _settings);