From 6f2ec2a77b5a384dcf36ef56eba2fe85bc1ce7ac Mon Sep 17 00:00:00 2001 From: C4 <81770958+code423n4@users.noreply.github.com> Date: Fri, 16 Dec 2022 07:44:14 +0100 Subject: [PATCH] immeas data for issue #216 --- data/immeas-Q.md | 197 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 data/immeas-Q.md diff --git a/data/immeas-Q.md b/data/immeas-Q.md new file mode 100644 index 0000000..b8607bb --- /dev/null +++ b/data/immeas-Q.md @@ -0,0 +1,197 @@ +# low + +## L-1 draw can be started with `redraw()` instead of `startDraw()` + +By transfering the raffled NFT to the contract manually a owner can start the raffle with `redraw()` instead of `startDraw()`. + +### Impact +This omits sending the `SetupDraw` event which could affect listeners to the protocol. It also breaks the intended usage flow. + +### Proof of Concept +PoC test in `VRFNFTRandomDraw.t.sol`: + +```javascript + function test_FullDrawingStartWithRedraw() public { + address winner = address(0x1337); + vm.label(winner, "winner"); + + vm.startPrank(winner); + for (uint256 tokensCount = 0; tokensCount < 10; tokensCount++) { + drawingNFT.mint(); + } + vm.stopPrank(); + + vm.startPrank(admin); + targetNFT.mint(); + + address consumerAddress = factory.makeNewDraw( + IVRFNFTRandomDraw.Settings({ + token: address(targetNFT), + tokenId: 0, + drawingToken: address(drawingNFT), + drawingTokenStartId: 0, + drawingTokenEndId: 10, + drawBufferTime: 1 hours, + recoverTimelock: block.timestamp + 1 weeks, + keyHash: bytes32( + 0x79d3d8832d904592c0bf9818b621522c988bb8b0c05cdc3b15aea1b6e8db0c15 + ), + subscriptionId: subscriptionId + }) + ); + vm.label(consumerAddress, "drawing instance"); + + mockCoordinator.addConsumer(subscriptionId, consumerAddress); + mockCoordinator.fundSubscription(subscriptionId, 100 ether); + + VRFNFTRandomDraw drawing = VRFNFTRandomDraw(consumerAddress); + targetNFT.setApprovalForAll(consumerAddress, true); + + // move nft to contract manually + targetNFT.transferFrom(admin, consumerAddress, 0); + // start with redraw instead of startDraw() + uint256 drawingId = drawing.redraw(); + + mockCoordinator.fulfillRandomWords(drawingId, consumerAddress); + vm.stopPrank(); + + vm.prank(winner); + drawing.winnerClaimNFT(); + assertEq(targetNFT.balanceOf(winner), 1); + assertEq(targetNFT.balanceOf(consumerAddress), 0); + } +``` + +### Tools Used +vs code, forge + +### Recommended Mitigation Steps +before doing a `redraw()`, verify that `currentChainlinkRequestId` is not empty. Then has to have been started before (through `startDraw()`). + +## L-2 weird comment + +https://github.com/code-423n4/2022-12-forgeries/blob/main/src/interfaces/IVRFNFTRandomDraw.sol#L64 + +```javascript +File: interfaces/IVRFNFTRandomDraw.sol + +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)) +``` + +## L-3 typos in comments + +https://github.com/code-423n4/2022-12-forgeries/blob/main/src/interfaces/IVRFNFTRandomDraw.sol#L46 + +```javascript +File: interfaces/IVRFNFTRandomDraw.sol + +46: /// @notice When the owner reclaims nft aftr recovery time delay +``` +_aftr_ + +https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L294 + +```javascript +File: VRFNFTRandomDraw.sol + +294: // Transfer token to the winter. +``` +_winter_ + + +## L-4 anyone can call `initialize()` + +There is no validation that it's the factory that created the contract that is calling `initialize()`. This could be achieved by adding immutable value with the creator in the constructor. + +# non-crit + +## NC-1 unreachable code #1 + +https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L144-L146 + +In `_requestRoll()`: +```javascript +141: function _requestRoll() internal { +142: // Chainlink request cannot be currently in flight. +143: // Request is cleared in re-roll if conditions are correct. +144: if (request.currentChainlinkRequestId != 0) { // can never be !=0 +145: revert REQUEST_IN_FLIGHT(); +146: } +``` + +This can never happen. `_requestRoll()` is called from two places: + +`startDraw()` which checks that `currentChainlinkRequestId != 0` before going into `_requestRoll()`: +```javascript +173: function startDraw() external onlyOwner returns (uint256) { +174: // Only can be called on first drawing +175: if (request.currentChainlinkRequestId != 0) { +176: revert REQUEST_IN_FLIGHT(); +177: } + +... + +182: // Request initial roll +183: _requestRoll(); +``` + +and in `redraw()` which just before does `delete request` which resets `currentChainlinkRequestId` to `0`: +```javascript +203: function redraw() external onlyOwner returns (uint256) { +204: if (request.drawTimelock >= block.timestamp) { +205: revert TOO_SOON_TO_REDRAW(); +206: } +207: +208: // Reset request +209: delete request; +210: +211: // Re-roll +212: _requestRoll(); +``` + +## NC-2 unreachable code #2 + +https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L149-L156 + +In `_requestRoll()`: +```javascript +203: if ( +204: request.hasChosenRandomNumber && +205: // Draw timelock not yet used +206: request.drawTimelock != 0 && +207: request.drawTimelock > block.timestamp +208: ) { +209: revert STILL_IN_WAITING_PERIOD_BEFORE_REDRAWING(); +210: } +``` + +This can never happen. `_requestRoll()` is called from these two places: + +`startDraw()`, already has a check for that `hasChosenRandomNumber` is `0`: + +```javascript +173: function startDraw() external onlyOwner returns (uint256) { +174: // Only can be called on first drawing +175: if (request.currentChainlinkRequestId != 0) { +176: revert REQUEST_IN_FLIGHT(); +177: } + +... + +182: // Request initial roll +183: _requestRoll(); +``` + +and in `redraw()` `request.drawTimelock` is checked already and it then does `delete request` which resets `currentChainlinkRequestId` to `0`: +```javascript +203: function redraw() external onlyOwner returns (uint256) { +204: if (request.drawTimelock >= block.timestamp) { +205: revert TOO_SOON_TO_REDRAW(); +206: } +207: +208: // Reset request +209: delete request; +210: +211: // Re-roll +212: _requestRoll(); +``` \ No newline at end of file