Platform: Code4rena
Start Date: 13/12/2022
Pot Size: $36,500 USDC
Total HM: 5
Participants: 77
Period: 3 days
Judge: gzeon
Total Solo HM: 1
Id: 191
League: ETH
Rank: 27/77
Findings: 2
Award: $71.66
🌟 Selected for report: 0
🚀 Solo Findings: 0
45.7078 USDC - $45.71
The design of the contract allows it to be raffled again, or even for the admin to claim it after a time that is configurable, and it can be brief, it does not make sense that the admin is not forced to send the token to the winner, in the case of that the winner does not make said call, since he may be indisposed, and not for that reason lose his draw. Also, since the admin is going to make the gas payment for the transfer, it doesn't make sense for the admin to receive it and not the winner.
Affected source code:
There is no token whitelist, so anyone can create an NFT token that they later destroy, or change ownership at the will of the contract owner. So it can be used to force people to buy a certain token (settings.drawingToken) and receive nothing in return.
There are three main considerations when using a timestamp to execute a critical function in a contract, especially when actions involve fund transfer.
block.timestamp.block.number as a timestamp
block.number property and average block time, however this is not future proof as block times may change (such as fork reorganisations and the difficulty bomb). In a sale spanning days, the 15-second rule allows one to achieve a more reliable estimate of time.
See SWC-116Reference:
Affected source code:
Some contracts do not implement good upgradeable logic.
Upgrading a contract will need a __gap storage in order to avoid storage collisions.
Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.
For example, the contract Version or VRFConsumerBaseV2 are inherited by upgradeable contracts, but these contracts don't have a __gap storage, so if a new variable is created it could lead to storage collisions that will produce a contract disaster, including loss of funds.
Reference:
Affected source code:
Recommended Mitigation Steps:
uint256[50] private __gap; to all upgradable contracts.The error messages indicate that it has to be greater than, but in the case of equals, it would not fail. It is recommended to correct the error message, or the logic.
// Check values in memory: - if (_settings.drawBufferTime < HOUR_IN_SECONDS) { + if (_settings.drawBufferTime <= HOUR_IN_SECONDS) { revert REDRAW_TIMELOCK_NEEDS_TO_BE_MORE_THAN_AN_HOUR(); } - if (_settings.drawBufferTime > MONTH_IN_SECONDS) { + if (_settings.drawBufferTime >= MONTH_IN_SECONDS) { revert REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH(); }
Affected source code:
abstract for base contractsabstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.
Reference:
Affected source code:
There is a typo error in the following line:
// Transfer token to the winter.
It must be winner.
Affected source code:
The pragma version used is:
pragma solidity 0.8.16;
The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:
Apart from these, there are several minor bug fixes and improvements.
Solidity contracts can use a special form of comments to provide rich documentation for functions, return variables and more. This special form is named the Ethereum Natural Language Specification Format (NatSpec).
It was detected that some methods doesn't added the return documentation, like the following ones:
makeNewDraw in VRFNFTRandomDrawFactory.sol:38 and IVRFNFTRandomDrawFactory.sol:22:/// @notice Function to make a new drawing /// @param settings settings for the new drawing + /// @return new draw address function makeNewDraw(IVRFNFTRandomDraw.Settings memory settings) external returns (address) {
hasUserWon in IVRFNFTRandomDraw.sol:108 and VRFNFTRandomDraw.sol:264:/// @notice Function to determine if the user has won in the current drawing /// @param user address for the user to check if they have won in the current drawing + /// @return tue if the user won function hasUserWon(address user) external view returns (bool);
Reference:
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:
Reference:
/// @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; /// @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; // @dev about 30 days in a month - uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30; + uint256 immutable MONTH_IN_SECONDS = 30 days;
Affected source code:
#0 - c4-judge
2022-12-17T17:24:18Z
gzeon-c4 marked the issue as grade-b
🌟 Selected for report: c3phas
Also found by: 0x1f8b, Aymen0909, Bnke0x0, Bobface, IllIllI, PaludoX0, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Sathish9098, adriro, chaduke, codeislight, ctrlc03, indijanc, izhelyazkov, kuldeep, nadin, neko_nyaa, nicobevi, rvierdiiev, shark
25.9485 USDC - $25.95
The following structures could be optimized moving the position of certain values in order to save some storage slots:
Settings in IVRFNFTRandomDraw.sol:71-91:
struct Settings { /// @notice Token Contract to put up for raffle address token; + uint64 subscriptionId; /// @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; }
Reference:
"before" in red, "after" in green. In general the deployment size size is larger, but the execution is cheaper.
| src/VRFNFTRandomDraw.sol:VRFNFTRandomDraw contract | | | | | | |----------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1237526 | 6717 | | | | | + | 1238926 | 6724 | | | | | | Function Name | min | avg | median | max | # calls | - | getRequestDetails | 572 | 572 | 572 | 572 | 3 | + | getRequestDetails | 550 | 550 | 550 | 550 | 3 | - | initialize | 43790 | 146546 | 175523 | 192923 | 15 | + | initialize | 41586 | 133672 | 153325 | 170725 | 15 | - | rawFulfillRandomWords | 1131 | 38496 | 46390 | 46390 | 6 | + | rawFulfillRandomWords | 1109 | 38474 | 46368 | 46368 | 6 | - | redraw | 572 | 28944 | 28944 | 57317 | 2 | + | redraw | 572 | 28951 | 28951 | 57331 | 2 | - | startDraw | 482 | 86573 | 96467 | 99775 | 9 | + | startDraw | 549 | 86569 | 96454 | 99762 | 9 | - | winnerClaimNFT | 422 | 11638 | 2422 | 27624 | 5 | + | winnerClaimNFT | 422 | 11606 | 2422 | 27546 | 5 | | src/VRFNFTRandomDrawFactory.sol:VRFNFTRandomDrawFactory contract | | | | | | |------------------------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1042203 | 5678 | | | | | + | 1044210 | 5688 | | | | | | Function Name | min | avg | median | max | # calls | - | acceptOwnership | 1966 | 1966 | 1966 | 1966 | 1 | + | acceptOwnership | 2019 | 2019 | 2019 | 2019 | 1 | - | contractVersion | 250 | 250 | 250 | 250 | 1 | + | contractVersion | 228 | 228 | 228 | 228 | 1 | - | initialize | 881 | 20519 | 27065 | 27065 | 4 | + | initialize | 926 | 20564 | 27110 | 27110 | 4 | - | makeNewDraw | 46872 | 183639 | 213232 | 239795 | 16 | + | makeNewDraw | 46944 | 171637 | 200269 | 217669 | 16 | - | owner | 376 | 376 | 376 | 376 | 3 | + | owner | 365 | 365 | 365 | 365 | 3 | - | proxiableUUID | 352 | 352 | 352 | 352 | 1 | + | proxiableUUID | 319 | 319 | 319 | 319 | 1 | - | safeTransferOwnership | 555 | 12443 | 12443 | 24331 | 2 | + | safeTransferOwnership | 600 | 12488 | 12488 | 24376 | 2 | - | upgradeTo | 845 | 4078 | 5555 | 5834 | 3 | + | upgradeTo | 823 | 4045 | 5500 | 5812 | 3 | | src/VRFNFTRandomDrawFactoryProxy.sol:VRFNFTRandomDrawFactoryProxy contract | | | | | | |----------------------------------------------------------------------------|-----------------|-------|--------|-------|---------| | Deployment Cost | Deployment Size | | | | | - | 192212 | 1850 | | | | | + | 192257 | 1850 | | | | | | Function Name | min | avg | median | max | # calls | - | acceptOwnership | 2280 | 2280 | 2280 | 2280 | 1 | + | acceptOwnership | 2332 | 2332 | 2332 | 2332 | 1 | - | makeNewDraw | 47315 | 47315 | 47315 | 47315 | 1 | + | makeNewDraw | 47387 | 47387 | 47387 | 47387 | 1 | - | owner | 771 | 771 | 771 | 771 | 2 | + | owner | 760 | 760 | 760 | 760 | 2 | - | safeTransferOwnership | 954 | 12840 | 12840 | 24726 | 2 | + | safeTransferOwnership | 999 | 12885 | 12885 | 24771 | 2 | - | upgradeTo | 1244 | 4474 | 5950 | 6230 | 3 | + | upgradeTo | 1222 | 4441 | 5895 | 6208 | 3 |
msg.senderfunction makeNewDraw(IVRFNFTRandomDraw.Settings memory settings) external returns (address) { - address admin = msg.sender; // Clone the contract address newDrawing = ClonesUpgradeable.clone(implementation); // Setup the new drawing - IVRFNFTRandomDraw(newDrawing).initialize(admin, settings); + IVRFNFTRandomDraw(newDrawing).initialize(msg.sender, settings); // Emit event for indexing - emit SetupNewDrawing(admin, newDrawing); + emit SetupNewDrawing(msg.sender, newDrawing); // Return address for integration or testing return newDrawing; }
Affected source code:
No negative impact was found, "before" in red, "after" in green
| src/VRFNFTRandomDraw.sol:VRFNFTRandomDraw contract | | | | | | |----------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1237526 | 6717 | | | | | + | 1227313 | 6666 | | | | | | Function Name | min | avg | median | max | # calls | - | winnerClaimNFT | 422 | 11638 | 2422 | 27624 | 5 | + | winnerClaimNFT | 419 | 11635 | 2419 | 27621 | 5 | | src/VRFNFTRandomDrawFactory.sol:VRFNFTRandomDrawFactory contract | | | | | | |------------------------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1042203 | 5678 | | | | | + | 1041403 | 5674 | | | | | | Function Name | min | avg | median | max | # calls | - | makeNewDraw | 46872 | 183639 | 213232 | 239795 | 16 | + | makeNewDraw | 46860 | 183631 | 213224 | 239783 | 16 | | src/VRFNFTRandomDrawFactoryProxy.sol:VRFNFTRandomDrawFactoryProxy contract | | | | | | |----------------------------------------------------------------------------|-----------------|-------|--------|-------|---------| | Function Name | min | avg | median | max | # calls | - | makeNewDraw | 47315 | 47315 | 47315 | 47315 | 1 | + | makeNewDraw | 47303 | 47303 | 47303 | 47303 | 1 |
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:
Avoiding the immutable variables we will have a cheaper deployment.
Reference:
uint256 immutable HOUR_IN_SECONDS = 60 * 60; => 1 hours; uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7); => 1 weeks; uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30; => 30 days;
Also it can be optimized the use of MONTH_IN_SECONDS in:
if ( _settings.recoverTimelock > - block.timestamp + (MONTH_IN_SECONDS * 12) + block.timestamp + 365 days ) { revert RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR(); }
Affected source code:
No negative impact was found, "before" in red, "after" in green
| src/VRFNFTRandomDraw.sol:VRFNFTRandomDraw contract | | | | | | |----------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1237526 | 6717 | | | | | + | 1205796 | 6495 | | | | | | Function Name | min | avg | median | max | # calls | - | initialize | 43790 | 146546 | 175523 | 192923 | 15 | + | initialize | 43790 | 146472 | 175431 | 192831 | 15 | | src/VRFNFTRandomDrawFactory.sol:VRFNFTRandomDrawFactory contract | | | | | | |------------------------------------------------------------------|-----------------|--------|--------|--------|---------| | Function Name | min | avg | median | max | # calls | - | makeNewDraw | 46872 | 183639 | 213232 | 239795 | 16 | + | makeNewDraw | 46872 | 183570 | 213140 | 239703 | 16 |
VRFNFTRandomDraw.initializeAvoid setting variables before using it, it will improve error executions.
/// @notice Initialize the contract with settings and an admin /// @param admin initial admin user /// @param _settings initial settings for draw function initialize(address admin, Settings memory _settings) public initializer { - // Set new settings - settings = _settings; // Check values in memory: if (_settings.drawBufferTime < HOUR_IN_SECONDS) { revert REDRAW_TIMELOCK_NEEDS_TO_BE_MORE_THAN_AN_HOUR(); } if (_settings.drawBufferTime > MONTH_IN_SECONDS) { revert REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH(); } if (_settings.recoverTimelock < block.timestamp + WEEK_IN_SECONDS) { revert RECOVER_TIMELOCK_NEEDS_TO_BE_AT_LEAST_A_WEEK(); } if ( _settings.recoverTimelock > block.timestamp + (MONTH_IN_SECONDS * 12) ) { revert RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR(); } // If NFT contract address is not a contract if (_settings.token.code.length == 0) { revert TOKEN_NEEDS_TO_BE_A_CONTRACT(_settings.token); } // If drawing token is not a contract if (_settings.drawingToken.code.length == 0) { revert TOKEN_NEEDS_TO_BE_A_CONTRACT(_settings.drawingToken); } // Validate token range: end needs to be greater than start // and the size of the range needs to be at least 2 (end is exclusive) if ( _settings.drawingTokenEndId < _settings.drawingTokenStartId || _settings.drawingTokenEndId - _settings.drawingTokenStartId < 2 ) { revert DRAWING_TOKEN_RANGE_INVALID(); } + // Set new settings + settings = _settings; // Setup owner as admin __Ownable_init(admin); ... }
Affected source code:
"before" in red, "after" in green.
| src/VRFNFTRandomDraw.sol:VRFNFTRandomDraw contract | | | | | | |----------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1237526 | 6717 | | | | | + | 1238126 | 6720 | | | | | | Function Name | min | avg | median | max | # calls | - | initialize | 43790 | 146546 | 175523 | 192923 | 15 | + | initialize | 23732 | 121280 | 175529 | 192929 | 15 | | src/VRFNFTRandomDrawFactory.sol:VRFNFTRandomDrawFactory contract | | | | | | |------------------------------------------------------------------|-----------------|--------|--------|--------|---------| | Function Name | min | avg | median | max | # calls | - | makeNewDraw | 46872 | 183639 | 213232 | 239795 | 16 | + | makeNewDraw | 46872 | 159952 | 213238 | 239801 | 16 |
The following check it will be done during the _requestRoll call, so it can be removed.
/// @notice Call this to start the raffle drawing /// @return chainlink request id function startDraw() external onlyOwner returns (uint256) { - // Only can be called on first drawing - if (request.currentChainlinkRequestId != 0) { - revert REQUEST_IN_FLIGHT(); - } // Emit setup draw user event emit SetupDraw(msg.sender, settings); // Request initial roll _requestRoll();
Affected source code:
"before" in red, "after" in green
Note that the increase of
Deployment SizeinstartDrawmust be a forge error, because it will remove code and logic, so it can't be more than before.
| src/VRFNFTRandomDraw.sol:VRFNFTRandomDraw contract | | | | | | |----------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1237526 | 6717 | | | | | + | 1230919 | 6684 | | | | | | Function Name | min | avg | median | max | # calls | - | startDraw | 482 | 86573 | 96467 | 99775 | 9 | + | startDraw | 5152 | 86985 | 96347 | 99655 | 9 |
constant instead of immutableIf the value won't be changed during the constructor, it's better to use a constant instead of an immutable because it's cheaper.
/// @notice Our callback is just setting a few variables, 200k should be more than enough gas. - uint32 immutable callbackGasLimit = 200_000; + uint32 constant callbackGasLimit = 200_000; /// @notice Chainlink request confirmations, left at the default - uint16 immutable minimumRequestConfirmations = 3; + uint16 constant minimumRequestConfirmations = 3; /// @notice Number of words requested in a drawing - uint16 immutable wordsRequested = 1; + uint16 constant wordsRequested = 1; /// @dev 60 seconds in a min, 60 mins in an hour - uint256 immutable HOUR_IN_SECONDS = 60 * 60; + uint256 constant HOUR_IN_SECONDS = 60 * 60; /// @dev 24 hours in a day 7 days in a week - uint256 immutable WEEK_IN_SECONDS = (3600 * 24 * 7); + uint256 constant WEEK_IN_SECONDS = (3600 * 24 * 7); // @dev about 30 days in a month - uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30; + uint256 constant MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;
Affected source code:
No negative impact was found, "before" in red, "after" in green
| src/VRFNFTRandomDraw.sol:VRFNFTRandomDraw contract | | | | | | |----------------------------------------------------|-----------------|--------|--------|--------|---------| | Deployment Cost | Deployment Size | | | | | - | 1237526 | 6717 | | | | | + | 1186680 | 6341 | | | | | | Function Name | min | avg | median | max | # calls | - | rawFulfillRandomWords | 1131 | 38496 | 46390 | 46390 | 6 | + | rawFulfillRandomWords | 1131 | 38491 | 46384 | 46384 | 6 | - | redraw | 572 | 28944 | 28944 | 57317 | 2 | + | redraw | 572 | 28937 | 28937 | 57303 | 2 | - | startDraw | 482 | 86573 | 96467 | 99775 | 9 | + | startDraw | 482 | 86557 | 96449 | 99757 | 9 |
#0 - c4-judge
2022-12-17T17:46:18Z
gzeon-c4 marked the issue as grade-b