Forgeries contest - RaymondFam's results

Unspecific compiler version pragma

The compiler version pragma of Version.sol is unspecific ^0.8.0. While this often makes sense for libraries to allow them to be included with multiple different versions of an application, it may be a security risk for the actual application implementation itself. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up actually checking a different EVM compilation that is ultimately deployed on the blockchain.

Avoid floating pragmas where possible. It is highly recommend pinning a concrete compiler version (latest without security issues) in at least the top-level “deployed” contracts to make it unambiguous which compiler version is being used. Rule of thumb: a flattened source-unit should have at least one non-floating concrete solidity compiler version pragma.

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

Here are the instances with missing NatSpec:

File: Version.sol

5:  uint32 private immutable __version;

13:  constructor(uint32 version) {

File: VRFNFTRandomDrawFactory.sol#L31-L34

    function initialize(address _initialOwner) initializer external {
        __Ownable_init(_initialOwner);
        emit SetupFactory();
    }

No storage gap for upgradeable contracts

Consider adding a storage gap at the end of an upgradeable contract, just in case it would entail some child contracts in the future. This would ensure no shifting down of storage in the inheritance chain.

Here is the contract instance entailed:

File: VRFNFTRandomDrawFactory.sol

+ uint256[50] private __gap;

Openzeppellin-upgrades-unsafe-allow constructor

As per Openzeppelin's recommendation:

https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/6

The guidelines are now to prevent front-running of initialize() on an implementation contract, by adding an empty constructor (if there has not been one) with the initializer modifier. Hence, the implementation contract gets initialized atomically upon deployment.

This feature is readily incorporated in the Solidity Wizard since the UUPS vulnerability discovery. You would just need to check UPGRADEABILITY to have a specific constructor code block added to the contract.

For instance, consider having the constructor instance below refactored as follows:

File: VRFNFTRandomDrawFactory.sol#L23-L29

    /// @notice Constructor to set the implementation
+   /// @custom:oz-upgrades-unsafe-allow constructor
    constructor(address _implementation) initializer {
+      _disableInitializers();
        if (_implementation == address(0)) {
            revert IMPL_ZERO_ADDRESS_NOT_ALLOWED();
        }
        implementation = _implementation;
    }

Empty Code Blocks

Empty code blocks should be commented with reason(s) for the emptiness, and/or emit an event or revert upon function calling. If not, it should be removed from the contract.

Here is an instance entailed:

File: VRFNFTRandomDrawFactory.sol#L55-L59

    function _authorizeUpgrade(address newImplementation)
        internal
        override
        onlyOwner
    {}

Empty events

Logs and events in Solidity are an important part of smart contract development —and critical infrastructure for each project. The following event has no parameter in it although some of the standard global parameters like block.timestamp, block.number etc would still entail along with the emission. Consider refactoring it by adding in some relevant and useful parameters.

File: VRFNFTRandomDrawFactory.sol#L33

        emit SetupFactory();

Incorrect Comment

The comment // Only can be called on first drawing is denoted in the code block of startDraw() in VRFNFTRandomDraw.sol:

File: VRFNFTRandomDraw.sol#L174-L177

        // Only can be called on first drawing
        if (request.currentChainlinkRequestId != 0) {
            revert REQUEST_IN_FLIGHT();
        }

However, the identical if block is also called when _requestRoll() is internally invoked both from startDraw() and redraw().

If this comment pertains to startDraw(), it should be grouped with the function NatSpec to avoid confusion.

Lines too long

Lines in source code are typically limited to 80 characters, but it’s reasonable to stretch beyond this limit when need be as monitor screens theses days are comparatively larger. Considering the files will most likely reside in GitHub that will have a scroll bar automatically kick in when the length is over 164 characters, all code lines and comments should be split when/before hitting this length. Keep line width to max 120 characters for better readability where possible.

Here is an instance entailed:

File: IVRFNFTRandomDraw.sol#L64

Note: This particular commented line will also need to be trimmed with its phrase, in case random number = 0 repeatedly nested in parenthesis.

Immutable variables should be parameterized in the constructor

Consider assigning immutable variables in the constructor via parameter inputs instead of directly assigning them with literal values. If the latter approach is preferred, consider declaring them as constants.

Additionally, consider adopting a standardized naming pattern on these immutable variables that are partly capitalized and partly uncapitalized.

Here are the instances entailed:

File: VRFNFTRandomDraw.sol#L22-L33

    uint32 immutable callbackGasLimit = 200_000;
    /// @notice Chainlink request confirmations, left at the default
    uint16 immutable minimumRequestConfirmations = 3;
    /// @notice Number of words requested in a drawing
    uint16 immutable wordsRequested = 1;

    /// @dev 60 seconds in a min, 60 mins in an hour
    uint256 immutable HOUR_IN_SECONDS = 60 * 60;
    /// @dev 24 hours in a day 7 days in a week
    uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7);
    // @dev about 30 days in a month
    uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

delete implication on associated if block

In redraw() of VRFNFTRandomDraw.sol, the delete code line resets all variables of the struct, request to their default respective values including request.hasChosenRandonNumber == false. For this reason, the second if block of _requestRoll() may be removed since the first condition is going to be always false when internally called by startDraw() and redraw().

File: VRFNFTRandomDraw.sol#L149-L156

-        if (
-            request.hasChosenRandomNumber &&
-            // Draw timelock not yet used
-            request.drawTimelock != 0 &&
-            request.drawTimelock > block.timestamp
-        ) {
-            revert STILL_IN_WAITING_PERIOD_BEFORE_REDRAWING();
-        }

Time units

According to:

https://docs.soliditylang.org/en/v0.8.14/units-and-global-variables.html

suffixes like seconds, minutes, hours, days and weeks after literal numbers can be used to specify units of time where seconds are the base unit and units are considered naively in the following way:

1 == 1 seconds 1 minutes == 60 seconds 1 hours == 60 minutes 1 days == 24 hours 1 weeks == 7 days

To avoid human error while making the assignment more verbose and distinctive, the following variable declarations may be rewritten as:

File: VRFNFTRandomDraw.sol#L28-L33

    /// @dev 60 seconds in a min, 60 mins in an hour
-    uint256 immutable HOUR_IN_SECONDS = 60 * 60;
+    uint256 immutable HOUR_IN_SECONDS = 1 hours; // or 60 minutes
    /// @dev 24 hours in a day 7 days in a week
-    uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7);
+    uint256 immutable WEEK_IN_SECONDS = 1 weeks; // or 7 days
    // @dev about 30 days in a month
-    uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;
+    uint256 immutable MONTH_IN_SECONDS = 30 days;

Rename the variables to better carry their implicit meaning like AN_HOUR, A_WEEK, and A_MONTH where deemed fit.

Redrawing the same winner

The contract logic could not prevent picking the same inactive winner again when redraw() is called, leading to another round of waiting. Consider increasing the lower limit of the drawing token range of 2 to something more practical like at least 5 or 10 to minimize the odds of bumping into this occurrence.

Checks should be as early as possible

The owner could have called redraw() of VRFNFTRandomDraw.sol right after the winner had claimed the NFT unknowingly or accidentally. For this reason, the second if block should be executed prior to invoking _requestRoll(). This will avoid a sizable amount of gas wastage pertaining to reverting an entire function logic of _requestRoll().

Unused imports

Importing source units that are not referenced in the contract increases the size of deployment. Consider removing them to save some gas. If there was a plan to use them in the future, a comment explaining why they were imported would be helpful.

Here are the instances entailed:

File: VRFNFTRandomDrawFactory.sol#L4

- import {IERC721EnumerableUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/IERC721EnumerableUpgradeable.sol";

File: VRFNFTRandomDraw.sol#L8

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

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: VRFNFTRandomDraw.sol#L173-L198

- 173:    function startDraw() external onlyOwner returns (uint256) {
     +    function startDraw() external onlyOwner returns (uint256 _requestId) {
...
- 197:        return request.currentChainlinkRequestId;
     +        _requestId = request.currentChainlinkRequestId;

Unneeded Cache

Caching a global variable, msg.sender is unnecessary as it has no gas saving benefit in doing so.

Here are the instances entailed:

File: VRFNFTRandomDrawFactory.sol#L42

        address admin = msg.sender;

File: VRFNFTRandomDraw.sol#L279

        address user = msg.sender;

|| costs less gas than its equivalent &&

Rule of thumb: (x && y) is (!(!x || !y))

Even with the 10k Optimizer enabled: ||, OR conditions cost less than their equivalent &&, AND conditions.

As an example, the instance below may be refactored as follows:

File: VRFNFTRandomDraw.sol#L149-L154

-        if (
+        if (!(
-            request.hasChosenRandomNumber &&
+            !request.hasChosenRandomNumber ||
            // Draw timelock not yet used
-            request.drawTimelock != 0 &&
+            request.drawTimelock == 0 ||
-            request.drawTimelock > block.timestamp
+            request.drawTimelock <= block.timestamp
-        ) {
+        )) {

Redundant if blocks

The following if block of startDraw() in VRFNFTRandomDraw.sol may be removed considering the identical check is going to be executed on lines 144 - 146 when internally calling _requestRoll(), to save gas both on contract deployment and function call:

File: VRFNFTRandomDraw.sol#L175-L177

-        if (request.currentChainlinkRequestId != 0) {
-            revert REQUEST_IN_FLIGHT();
-        }

Payable access control functions costs less gas

Consider marking functions with access control as payable. This will save 20 gas on each call by their respective permissible callers for not needing to have the compiler check for msg.value.

For instance, the code line below may be refactored as follows:

File: VRFNFTRandomDraw.sol#L173

-    function startDraw() external onlyOwner returns (uint256) {
+    function startDraw() external payable onlyOwner returns (uint256) {

Memory variable not utilized when emitting event

In VRFNFTRandomDraw.sol, the emitted event is found to be using the state variable equivalent instead of the stack memory variable. Consider having the affected code line refactored as follows to save more gas as on the function call:

File: VRFNFTRandomDraw.sol#L75-L138

- 123:        emit InitializedDraw(msg.sender, settings);
     +        emit InitializedDraw(msg.sender, _settings);

State Variables Repeatedly Read Should be Cached

SLOADs cost 100 gas each after the 1st one whereas MLOADs/MSTOREs only incur 3 gas each. As such, storage values read multiple times should be cached in the stack memory the first time (costing only 1 SLOAD) and then re-read from this cache to avoid multiple SLOADs.

For instance, settings in the code block below should be cached as follows:

File: VRFNFTRandomDraw.sol#L277-L300

+       Settings memory _settings = settings // SLOAD 1
        emit WinnerSentNFT(
            user,
-            address(settings.token),
+            address(_settings.token), // MLOAD 1
-            settings.tokenId,
+            _settings.tokenId, // MLOAD 2
-            settings
+            _settings // MLOAD 3
        );

        // Transfer token to the winter.
-        IERC721EnumerableUpgradeable(settings.token).transferFrom(
+        IERC721EnumerableUpgradeable(_settings.token).transferFrom( // MLOAD 4
            address(this),
            msg.sender,
-            settings.tokenId
+            _settings.tokenId // MLOAD 5
        );

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: OwnableUpgradeable.sol#L44-L49

+    function _onlyPendingOwner() private view {
+        if (msg.sender != _pendingOwner) {
+            revert ONLY_PENDING_OWNER();
+        }
+    }

    modifier onlyPendingOwner() {
-        if (msg.sender != _pendingOwner) {
-            revert ONLY_PENDING_OWNER();
-        }
+        _onlyPendingOwner();
        _;
    }

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
  version: "0.8.16",
  settings: {
    optimizer: {
      enabled: true,
      runs: 1000,
    },
  },
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.

Unchecked SafeMath saves gas

"Checked" math, which is default in ^0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive than the current SafeMath, one should keep in mind that these checks will increase the cost of "basic math operation" that were not previously covered. This particularly concerns variable increments in for loops. When no arithmetic overflow/underflow is going to happen, unchecked { ... } to use the previous wrapping behavior further saves gas just as in the instance entailed below, as an example:

File: VRFNFTRandomDraw.sol#L249-L256

+ unchecked {
        uint256 tokenRange = settings.drawingTokenEndId -
            settings.drawingTokenStartId;

        // Store a number from it here (reduce number here to reduce gas usage)
        // We know there will only be 1 word sent at this point.
        request.currentChosenTokenId =
            (_randomWords[0] % tokenRange) +
            settings.drawingTokenStartId;
+ }