Forgeries contest - Aymen0909's results

QA Report

Summary

Findings

1- Mistake when calculating MONTH_IN_SECONDS value will allow bigger max values for _settings.drawBufferTime and _settings.recoverTimelock :

When calculating the value of MONTH_IN_SECONDS which is supposed to represent amount of seconds in a month, the formula used contain an error so instead of getting the equivalent of 1 month in seconds the code will calculate the equivalent of 7 months in seconds.

This will allow a bigger maximum value for both _settings.drawBufferTime and _settings.recoverTimelock variables which can possibly impact the protocol working in the futur.

Risk : Low

Proof of Concept

The issue occurs here :

File: src/VRFNFTRandomDraw.sol Line 32-33

// @dev about 30 days in a month
uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

The above formula calculate the equivalent of 7 months in seconds as 1 month == 3600 * 24 * 30

And the MONTH_IN_SECONDS is used later in the initialize function function to check the max values of the variables _settings.drawBufferTime and _settings.recoverTimelock :

File: src/VRFNFTRandomDraw.sol Line 86-88

if (_settings.drawBufferTime > MONTH_IN_SECONDS) {
    revert REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH();
}

File: src/VRFNFTRandomDraw.sol Line 93-98

if (
    _settings.recoverTimelock >
    block.timestamp + (MONTH_IN_SECONDS * 12)
) {
    revert RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR();
}

This will eventuly allow bigger maximum values for both variabales which doesn't have big impact on the current contract logic (can only cause longer wait for redraw or admin NFT reclaim) but it can cause problems if this contract is used in interaction with part of the protocol.

Mitigation

The solve this issue correct the formula of MONTH_IN_SECONDS as follow :

// @dev about 30 days in a month
uint256 immutable MONTH_IN_SECONDS = 3600 * 24 * 30;

2- Front-runable initialize function :

The initialize function is used in VRFNFTRandomDrawFactory contract to initialize the contract owner but there is the issue that any attacker can initialize the contract before the legitimate deployer and even if the developers when deploying call immediately the initialize function, malicious agents can trace the protocol deployment transactions and insert their own transaction between them and by doing so they front run the developers call and gain the ownership of the contract.

The impact of this issue is :

• In the best case developers notice the problem and have to redeploy the contract and thus costing more gas.

• In the worst case the protocol continue to work with the wrong owner which could lead to various problems for users.

Risk : Low

Proof of Concept

Instances include:

File: src/VRFNFTRandomDrawFactory.sol Line 31-34

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

Mitigation

It's recommended to use the constructor to initialize non-proxied contracts. But in this case for initializing proxy (upgradable) contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify that the transaction succeeded.

3- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address) is not the address(0)

Risk : Low

Proof of Concept

Instances include:

File: src/VRFNFTRandomDraw.sol Line 51

coordinator = _coordinator;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

4- Event should be emitted in the redraw() function :

The redraw() function is used to reset the giveaway process in case the current winner doesn't reclaim it's NFT, thus the fonction should emit an event so that Dapps and users can know that a new request has been made to choose a new winner.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: src/VRFNFTRandomDraw.sol

function redraw() external onlyOwner returns (uint256)

Mitigation

Emit an event in the redraw() function.

5- Use constant instead of immutable for variables that are not set in constructor :

In solidity best practices constant keyword is used for variables which don't change in value after contract compilation and the immutable keyword is used for variables that are set only in the constructor, and as all the immutable variables used in VRFNFTRandomDraw are not set in constructor they should be marked constant.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: src/VRFNFTRandomDraw.sol Line 21-33

/// @notice Our callback is just setting a few variables, 200k should be more than enough gas.
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;

Mitigation

Replace immutable by constant in the aforementioned variables.

6- Use solidity time-based units :

Solidity contains time-based units (seconds, minutes, hours, days and weeks) which should be used when declaring time based variables/constants instead of using literal numbers, this is done for better readability and to avoid mistakes.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: src/VRFNFTRandomDraw.sol Line 28-33

/// @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;

Mitigation

Use time units when declaring time-based variables.

Gas Optimizations

Summary

Findings

1- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, 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 are 2 instances of this issue:

File: src/interfaces/IVRFNFTRandomDraw.sol Line 59-68

struct CurrentRequest {
    /// @notice current chainlink request id
    uint256 currentChainlinkRequestId;
    /// @notice current chosen random number
    uint256 currentChosenTokenId;
    /// @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))
    bool hasChosenRandomNumber;
    /// @notice time lock (block.timestamp) that a re-draw can be issued
    uint256 drawTimelock;
}

As the drawTimelock variable represents time unit (in seconds) it can't really overflow 2^128 (wich give 1.0783e+31 years when converted), and the currentChosenTokenId value is always ranged between drawingTokenStartId and drawingTokenEndId (bcause those two values represent real NFT token IDs they can't overflow in reality) so it can't really overflow 2^128, those variables values should be packed together and the struct should be modified as follow to save gas :

struct CurrentRequest {
    /// @notice current chainlink request id
    uint256 currentChainlinkRequestId;
    /// @notice time lock (block.timestamp) that a re-draw can be issued
    uint128 drawTimelock;
    /// @notice current chosen random number
    uint128 currentChosenTokenId;
    /// @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))
    bool hasChosenRandomNumber;
}

File: src/interfaces/IVRFNFTRandomDraw.sol Line 71-90

struct Settings {
    /// @notice Token Contract to put up for raffle
    address token;
    /// @notice Token ID to put up for raffle
    uint256 tokenId;
    /// @notice Token that each (sequential) ID has a entry in the raffle.
    address drawingToken;
    /// @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)
    uint256 drawingTokenStartId;
    /// @notice End token ID for the drawing (exclusive) (token ids 0 - 9 would be 10 in this field)
    uint256 drawingTokenEndId;
    /// @notice Draw buffer time – time until a re-drawing can occur if the selected user cannot or does not claim the NFT.
    uint256 drawBufferTime;
    /// @notice block.timestamp that the admin can recover the NFT (as a safety fallback)
    uint256 recoverTimelock;
    /// @notice Chainlink gas keyhash
    bytes32 keyHash;
    /// @notice Chainlink subscription id
    uint64 subscriptionId;
}

Here we have the following :

The variabales tokenId, drawingTokenStartId and drawingTokenEndId represent real NFT collection ids so they can't realy overflow 2^64 as most NFT collection have a max supply and even if they don't the token id can't realistically be greater than 2^64 = 1.8446744e+19

The variabales drawBufferTime and recoverTimelock represent time unit and they have both upper and lower bond :

HOUR_IN_SECONDS <= drawBufferTime <= MONTH_IN_SECONDS

time + WEEK_IN_SECONDS <= recoverTimelock <= time + 1 year

Thus those two values can't realistically overflow 2^64 (which is equivalent to 584554046918.09 year)

So all the previously mentionned variabales should be packed together to save gas and the struct should be modified as follow :

struct Settings {
    /// @notice Token Contract to put up for raffle
    address token;
    /// @notice Token that each (sequential) ID has a entry in the raffle.
    address drawingToken;
    /// @notice Token ID to put up for raffle
    uint64 tokenId;
    /// @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)
    uint64 drawingTokenStartId;
    /// @notice End token ID for the drawing (exclusive) (token ids 0 - 9 would be 10 in this field)
    uint64 drawingTokenEndId;
    /// @notice Draw buffer time – time until a re-drawing can occur if the selected user cannot or does not claim the NFT.
    uint64 drawBufferTime;
    /// @notice block.timestamp that the admin can recover the NFT (as a safety fallback)
    uint64 recoverTimelock;
    /// @notice Chainlink subscription id
    uint64 subscriptionId;
    /// @notice Chainlink gas keyhash
    bytes32 keyHash;
}

2- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 2 instances of this issue:

File: src/VRFNFTRandomDraw.sol Line 159

request.drawTimelock = block.timestamp + settings.drawBufferTime; 

The above operation cannot overflow because request.drawTimelock represent a time measurement (in seconds) and we are adding to the current timestamp a bounded value settings.drawBufferTime as illustated here thus it can't realistically overflow so the operation should be marked unchecked.

File: src/VRFNFTRandomDraw.sol Line 249

uint256 tokenRange = settings.drawingTokenEndId - settings.drawingTokenStartId;

The above operation cannot underflow because of the check :

File: src/VRFNFTRandomDraw.sol Line 112

if (
    _settings.drawingTokenEndId < _settings.drawingTokenStartId ||
    _settings.drawingTokenEndId - _settings.drawingTokenStartId < 2
) {
    revert DRAWING_TOKEN_RANGE_INVALID();
}

So the operation should be marked unchecked.

3- storage variables should be cached into memory variables instead of re-reading them from storage :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 4 instances of this issue :

File: src/VRFNFTRandomDraw.sol Line 149-154

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

The value of request.drawTimelock is read twice from storage, the above code should be modified as follow to save gas :

uint256 drawTimeLock = request.drawTimelock;
if (
    request.hasChosenRandomNumber &&
    // Draw timelock not yet used
    drawTimeLock != 0 &&
    drawTimeLock > block.timestamp
)   

File: src/VRFNFTRandomDraw.sol Line 249-256

// Get total token range
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;

The value of settings.drawingTokenStartId is read twice from storage, the above code should be modified as follow to save gas :

uint256 startId = settings.drawingTokenStartId;

// Get total token range
uint256 tokenRange = settings.drawingTokenEndId - startId;

// 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) + startId;    

File: src/VRFNFTRandomDraw.sol Line 287-299

emit WinnerSentNFT(
    user,
    address(settings.token),
    settings.tokenId,
    settings
);

// Transfer token to the winter.
IERC721EnumerableUpgradeable(settings.token).transferFrom(
    address(this),
    msg.sender,
    settings.tokenId
);   

The values of settings.token and settings.tokenId are read twice from storage, the above code should be modified as follow to save gas :

address token = settings.token;
uint256 tokenId = settings.tokenId;

emit WinnerSentNFT(
    user,
    token,
    tokenId,
    settings
);

// Transfer token to the winter.
IERC721EnumerableUpgradeable(token).transferFrom(
    address(this),
    msg.sender,
    tokenId
);     

File: src/VRFNFTRandomDraw.sol Line 312-319

// Send event for indexing that the owner reclaimed the NFT
emit OwnerReclaimedNFT(owner());

// Transfer token to the admin/owner.
IERC721EnumerableUpgradeable(settings.token).transferFrom(
    address(this),
    owner(),
    settings.tokenId
); 

The owner() method is called twice and it will read the owner address twice from storage which costs more gas, and because here the lastResortTimelockOwnerClaimNFT() function can only be called by the owner so we will always have msg.sender == owner(), the above code should be modified as follow to save gas :

address owner = msg.sender;

// Send event for indexing that the owner reclaimed the NFT
emit OwnerReclaimedNFT(owner);

// Transfer token to the admin/owner.
IERC721EnumerableUpgradeable(settings.token).transferFrom(
    address(this),
    owner,
    settings.tokenId
);     

4- memory values should be emitted in events instead of storage ones :

Here, the values emitted shouldn’t be read from storage. The existing memory values should be used instead, this will save ~101 GAS

There are 2 instances of this issue :

File: src/VRFNFTRandomDraw.sol Line 123

// Emit initialized event for indexing
emit InitializedDraw(msg.sender, settings);

The storage settings variable is emitted which cost more gas, should instead emit memory variables _settings as follow :

// Emit initialized event for indexing
emit InitializedDraw(msg.sender, _settings);

File: src/VRFNFTRandomDraw.sol Line 312

// Send event for indexing that the owner reclaimed the NFT
emit OwnerReclaimedNFT(owner());

The owner() method read from storage the value of owner address but as the lastResortTimelockOwnerClaimNFT() function can only be called by the owner so we will always have msg.sender == owner(), and thus the msg.sender value should be emitted to save gas :

// Send event for indexing that the owner reclaimed the NFT
emit OwnerReclaimedNFT(msg.sender);