Lines of code

Vulnerability details

Impact

The admin/owner of VRFNFTRandomDraw can wait for recoverTimelock to expire before making the draw. This way he can use lastResortTimelockOwnerClaimNFT() to take back the reward NFT from the contract without any time to allow for the winner to claim. He could also avoid the redraw timelock this way by taking back the NFT and creating another Draw. The protocol does mention they’re not considering Draw owners/admins to be malicious but would still consider this at least a medium because the rugpull vector can easily be prevented within the contract.

Proof of Concept

This can be quickly verified with modifying one of the protocol tests - test_FullDrawing():

diff --git a/test/VRFNFTRandomDraw.t.sol b/test/VRFNFTRandomDraw.t.sol
index a023626..2697a10 100644
--- a/test/VRFNFTRandomDraw.t.sol
+++ b/test/VRFNFTRandomDraw.t.sol
@@ -338,10 +338,19 @@ address sender = address(0x994);

         targetNFT.setApprovalForAll(consumerAddress, true);

+        // Owner waits for the recover timelock before starting the draw
+        vm.warp(block.timestamp + 2 weeks);
+
         uint256 drawingId = drawing.startDraw();

         mockCoordinator.fulfillRandomWords(drawingId, consumerAddress);

+        // After getting random word and determining winner admin changes his mind and claims the NFT
+        drawing.lastResortTimelockOwnerClaimNFT();
+        // Assert that we're still within the draw timelock
+        (,,, uint256 drawTimelock) = drawing.request();
+        assert(drawTimelock > block.timestamp);
+
         vm.stopPrank();

         assertEq(targetNFT.balanceOf(winner), 0);

Notice the test fails because winner cannot claim the NFT even though we’re still within the draw timelock.

Tools Used

Manual review

Recommended Mitigation Steps

A couple of options to mitigate this.

1. Include the check that we’re not within draw timelock inside lastResortTimelockOwnerClaimNFT() to allow for the winner to claim his NFT:

   function lastResortTimelockOwnerClaimNFT() external onlyOwner {
   // If recoverTimelock is not setup, or if not yet occurred
   if (settings.recoverTimelock > block.timestamp || request.drawTimelock > block.timestamp) {
   // Stop the withdraw
   revert RECOVERY_IS_NOT_YET_POSSIBLE();
   }

2. Extend the settings.recoverTimelock every time the startDraw() or redraw() are called in a similar manner to the request.drawTimelock recalculation.

The text was updated successfully, but these errors were encountered:

All reactions