Forgeries contest - Rolezn's results

Summary<a name="Summary">

Low Risk Issues

Total: 1 contexts over 1 issues

Non-critical Issues

Total: 10 contexts over 7 issues

Low Risk Issues

<a href="#Summary">[LOW‑1]</a><a name="LOW&#x2011;1"> Contracts are not using their OZ Upgradeable counterparts

The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.

<ins>Proof of Concept</ins>

4: import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDrawFactoryProxy.sol#L4

<ins>Recommended Mitigation Steps</ins>

Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.

Non Critical Issues

<a href="#Summary">[NC‑1]</a><a name="NC&#x2011;1"> Use a more recent version of Solidity

<a href="https://blog.soliditylang.org/2021/04/21/solidity-0.8.4-release-announcement/">0.8.4</a>: bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)

<a href="https://blog.soliditylang.org/2022/02/16/solidity-0.8.12-release-announcement/">0.8.12</a>: string.concat() instead of abi.encodePacked(<str>,<str>)

<a href="https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/">0.8.13</a>: Ability to use using for with a list of free functions

<a href="https://blog.soliditylang.org/2022/05/18/solidity-0.8.14-release-announcement/">0.8.14</a>:

ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions.

<a href="https://blog.soliditylang.org/2022/06/15/solidity-0.8.15-release-announcement/">0.8.15</a>:

Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

<a href="https://blog.soliditylang.org/2022/08/08/solidity-0.8.16-release-announcement/">0.8.16</a>:

Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

<a href="https://blog.soliditylang.org/2022/09/08/solidity-0.8.17-release-announcement/">0.8.17</a>:

Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

<ins>Proof Of Concept</ins>

pragma solidity ^0.8.10;

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/utils\Version.sol#L2

<ins>Recommended Mitigation Steps</ins>

Consider updating to a more recent solidity version.

<a href="#Summary">[NC‑2]</a><a name="NC&#x2011;2"> 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.

<ins>Proof Of Concept</ins>

function resignOwnership() public onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L114

<a href="#Summary">[NC‑3]</a><a name="NC&#x2011;3"> Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

<ins>Proof Of Concept</ins>

13: constructor(uint32 version)

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/utils\Version.sol#L13

<a href="#Summary">[NC‑4]</a><a name="NC&#x2011;4"> Avoid Floating Pragmas: The Version Should Be Locked

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

<ins>Proof Of Concept</ins>

Found usage of floating pragmas ^0.8.10 of Solidity in [Version.sol]

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/utils\Version.sol#L2

<a href="#Summary">[NC‑5]</a><a name="NC&#x2011;5"> Use of Block.Timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts. References: SWC ID: 116

<ins>Proof Of Concept</ins>

90: if (_settings.recoverTimelock < block.timestamp + WEEK_IN_SECONDS) {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L90

153: request.drawTimelock > block.timestamp

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L153

204: if (request.drawTimelock >= block.timestamp) {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L204

306: if (settings.recoverTimelock > block.timestamp) {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L306

<ins>Recommended Mitigation Steps</ins>

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

<a href="#Summary">[NC‑6]</a><a name="NC&#x2011;6"> 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 Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

<ins>Proof Of Concept</ins>

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/tree/main/src/interfaces\IVRFNFTRandomDraw.sol#L64

<a href="#Summary">[NC‑7]</a><a name="NC&#x2011;7"> Event emit should emit a parameter

Some emitted events do not have any emitted parameters. It is recommended to add some parameter such as state changes or value changes when events are emitted

<ins>Proof Of Concept</ins>

33: emit SetupFactory()

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDrawFactory.sol#L33

Summary<a name="Summary">

Gas Optimizations

Total: 30 contexts over 8 issues

Gas Optimizations

<a href="#Summary">[GAS‑1]</a><a name="GAS&#x2011;1"> Empty Blocks Should Be Removed Or Emit Something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified (if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}})

<ins>Proof Of Concept</ins>

192: {} catch {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L192

59: {}

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDrawFactory.sol#L59

<a href="#Summary">[GAS‑2]</a><a name="GAS&#x2011;2"> Public Functions To External

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

<ins>Proof Of Concept</ins>

function hasUserWon(address user) public view returns (bool) {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L264

function owner() public view virtual returns (address) {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L68

function pendingOwner() public view returns (address) {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L73

function resignOwnership() public onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L114

function acceptOwnership() public onlyPendingOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L119

function cancelOwnershipTransfer() public onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L128

<a href="#Summary">[GAS‑3]</a><a name="GAS&#x2011;3"> Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

<ins>Proof Of Concept</ins>

22: uint32 immutable callbackGasLimit = 200_000;

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L22

24: uint16 immutable minimumRequestConfirmations = 3;

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L24

26: uint16 immutable wordsRequested = 1;

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L26

89: uint64 subscriptionId;

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/interfaces\IVRFNFTRandomDraw.sol#L89

5: uint32 private immutable __version;

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/utils\Version.sol#L5

<a href="#Summary">[GAS‑4]</a><a name="GAS&#x2011;4"> Optimize names to save gas

Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions.

See more <a href="https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92">here</a>

<ins>Proof Of Concept</ins>

File: .\Projects\forgeries\2022-12-forgeries\src\VRFNFTRandomDrawFactoryProxy.sol

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDrawFactoryProxy.sol

File: .\Projects\forgeries\2022-12-forgeries\src\ownable\OwnableUpgradeable.sol

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol

File: .\Projects\forgeries\2022-12-forgeries\src\utils\Version.sol

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/utils\Version.sol

<ins>Recommended Mitigation Steps</ins>

Find a lower method ID name for the most called functions for example Call() vs. Call1() is cheaper by 22 gas For example, the function IDs in the Gauge.sol contract will be the most used; A lower method ID may be given.

<a href="#Summary">[GAS‑5]</a><a name="GAS&#x2011;5"> Use uint256(1)/uint256(2) instead for true and false boolean states

If you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from true to false can cost up to ~20000 gas rather than uint256(2) to uint256(1) that would cost significantly less.

<ins>Proof Of Concept</ins>

246: request.hasChosenRandomNumber = true;

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L246

<a href="#Summary">[GAS‑6]</a><a name="GAS&#x2011;6"> internal functions only called once can be inlined to save gas

<ins>Proof Of Concept</ins>

230: function fulfillRandomWords

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L230

<a href="#Summary">[GAS‑7]</a><a name="GAS&#x2011;7"> Setting the constructor to payable

Saves ~13 gas per instance

<ins>Proof Of Concept</ins>

47: constructor(VRFCoordinatorV2Interface _coordinator)
        VRFConsumerBaseV2(address(_coordinator))
        initializer

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L47

24: constructor(address _implementation) initializer

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDrawFactory.sol#L24

12: constructor(address _logic, address _defaultOwner)
        ERC1967Proxy(
            _logic,
            abi.encodeWithSelector(
                IVRFNFTRandomDrawFactory.initialize.selector,
                _defaultOwner
            )
        )

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDrawFactoryProxy.sol#L12

13: constructor(uint32 version)

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/utils\Version.sol#L13

<a href="#Summary">[GAS‑8]</a><a name="GAS&#x2011;8"> Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier or require such as onlyOwner/onlyX 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.

<ins>Proof Of Concept</ins>

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

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

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

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L203

304: function lastResortTimelockOwnerClaimNFT() external onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/VRFNFTRandomDraw.sol#L304

79: function transferOwnership(address _newOwner)
        public
        notZeroAddress(_newOwner)
        onlyOwner
    {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L79

102: function safeTransferOwnership(address _newOwner)
        public
        notZeroAddress(_newOwner)
        onlyOwner
    {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L102

114: function resignOwnership() public onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L114

119: function acceptOwnership() public onlyPendingOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L119

128: function cancelOwnershipTransfer() public onlyOwner {

https://github.com/code-423n4/2022-12-forgeries/tree/main/src/ownable\OwnableUpgradeable.sol#L128

<ins>Recommended Mitigation Steps</ins>

Functions guaranteed to revert when called by normal users can be marked payable.