Forgeries contest - 0xAgro's results

QA Report

Finding Summary

[NC-01] Long Lines (> 120 Characters)

Lines with greater length than 120 characters are used. The Solidity Style Guide suggests that all lines should be 120 characters or less in width.

Findings:

/src/VRFNFTRandomDrawFactory.sol Links: 4.

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

/src/VRFNFTRandomDraw.sol Links: 5.

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

/src/interfaces/IVRFNFTRandomDraw.sol Links: 64, 78, 82.

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))
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)
82:	        /// @notice Draw buffer time – time until a re-drawing can occur if the selected user cannot or does not claim the NFT.

[NC-02] Order of Functions Not Compliant With Solidity Docs

The Solidity Style Guide suggests the following function ordering: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

Findings:

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

Trading.sol: the constructor is positioned after an external function. VRFNFTRandomDraw.sol: external functions are positioned after internal functions. OwnableUpgradeable.sol: public functions are positioned after internal functions.

[NC-03] Order of Functions Not Compliant With Solidity Docs

The Solidity Style Guide suggests the following modifer ordering: Visibility, Mutability, Virtual, Override, Custom modifiers.

Findings:

initialize has a custom modifier after a visibility modifier.

/src/VRFNFTRandomDrawFactory.sol Links: 31

31:	function initialize(address _initialOwner) initializer external {

[NC-04] NatSpec Comments Use // Instead of ///

The NatSpec notation requires the use of /// or /** to differentiate from regular comments.

Findings:

/src/VRFNFTRandomDraw.sol Links: 32.

32:	// @dev about 30 days in a month

[NC-05] Inconsistent . In Comments

Most comments in the codebase do not end with a .. It is best to keep a consistent style.

Findings:

/src/VRFNFTRandomDraw.sol Links: 21, 39, 142, 143, 253, 278, 281, 294.

21:	/// @notice Our callback is just setting a few variables, 200k should be more than enough gas.
39:	/// @notice Settings used for the contract.
142:	// Chainlink request cannot be currently in flight.
143:	// Request is cleared in re-roll if conditions are correct.
253:	// We know there will only be 1 word sent at this point.
278:	// Assume (potential) winner calls this fn, cache.
281:	// Check if this user has indeed won.
294:	// Transfer token to the winter.

/src/interfaces/IVRFNFTRandomDraw.sol Links: 76, 82.

76:	/// @notice Token that each (sequential) ID has a entry in the raffle.
82:	/// @notice Draw buffer time – time until a re-drawing can occur if the selected user cannot or does not claim the NFT.

/src/interfaces/IVRFNFTRandomDrawFactory.sol Links: 7.

7:	/// @notice Cannot be initialized with a zero address impl.

/src/ownable/OwnableUpgradeable.sol Links: 89, 113.

89:	/// @dev Ensure is called only from trusted internal code, no access control checks.
113:	/// @dev only callably by the owner, dangerous call.

[NC-06] Misleading Comment / Possible Typo

There is a seemingly odd comment that appears to have been copy and pasted by accident multiple times. Consider describing the comment in more detail if intentional or remove the excess if not.

Findings:

/src/interfaces/IVRFNFTRandomDraw.sol Links: 64.

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))

[NC-07] Winter - Winner Typo

Based on other comments and context in the winnerClaimNFT function it seems as though Winner was misspelled as Winter on L294.

Findings:

/src/VRFNFTRandomDraw.sol Links: 294.

294:	// Transfer token to the winter.

Suggested Change

294:	// Transfer token to the winner.