Forgeries contest - hihen's results

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L306 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L203-L206

Vulnerability details

Impact

The owner of VRFNFTRandomDraw can fool users/winner by start a valid draw in which no one can claim the winning NFT.

Proof of Concept

The protocol tries to ensure that the winner has enough time to claim the winning NFT once the draw starts by doing the following checks in VRFNFTRandomDraw.initialize():

if (_settings.drawBufferTime < HOUR_IN_SECONDS) {
    revert REDRAW_TIMELOCK_NEEDS_TO_BE_MORE_THAN_AN_HOUR();
}
...
if (_settings.recoverTimelock < block.timestamp + WEEK_IN_SECONDS) {
    revert RECOVER_TIMELOCK_NEEDS_TO_BE_AT_LEAST_A_WEEK();
}

However, this does not effectively guarantee that the winner will always have a chance to claim the winning NFT.

Here is an example of an owner starting a draw which the winner will never be able to claim the winning NFT:

1. X make a new draw with: drawBufferTime = 1 day, recoverTimelock = 1 week, token = PUNK
2. 1 week later, X calls startDraw() and immediately calls lastResortTimelockOwnerClaimNFT().

owner X will claim the NFT successfully because block time has passed recoverTimelock

3. Some blocks later, the winner was chosen in fulfillRandomWords().
4. Winner calls winnerClaimNFT() trying to claim the PUNK, but the transaction always fails because the winning NFT

Tools Used

VS Code

Recommended Mitigation Steps

One of the following two options is recommended:

1. Add additional checks to VRFNFTRandomDraw.initialize() and VRFNFTRandomDraw.startDraw():

diff --git a/src/VRFNFTRandomDraw.sol b/src/VRFNFTRandomDraw.sol
index 668bc56..54a141b 100644
--- a/src/VRFNFTRandomDraw.sol
+++ b/src/VRFNFTRandomDraw.sol
@@ -97,6 +97,12 @@ contract VRFNFTRandomDraw is
             revert RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR();
         }

+        if (
+            _settings.recoverTimelock < block.timestamp + _settings.drawBufferTime + HOUR_IN_SECONDS
+        ) {
+            revert RECOVER_TIMELOCK_SHOULD_BE_LATER();
+        }
+
         // If NFT contract address is not a contract
         if (_settings.token.code.length == 0) {
             revert TOKEN_NEEDS_TO_BE_A_CONTRACT(_settings.token);
@@ -176,6 +182,12 @@ contract VRFNFTRandomDraw is
             revert REQUEST_IN_FLIGHT();
         }

+        if (
+            settings.recoverTimelock < block.timestamp + settings.drawBufferTime + HOUR_IN_SECONDS
+        ) {
+            revert RECOVER_TIMELOCK_SHOULD_BE_LATER();
+        }
+
         // Emit setup draw user event
         emit SetupDraw(msg.sender, settings);

diff --git a/src/interfaces/IVRFNFTRandomDraw.sol b/src/interfaces/IVRFNFTRandomDraw.sol
index 4775288..5418f95 100644
--- a/src/interfaces/IVRFNFTRandomDraw.sol
+++ b/src/interfaces/IVRFNFTRandomDraw.sol
@@ -30,6 +30,8 @@ interface IVRFNFTRandomDraw {
     error RECOVER_TIMELOCK_NEEDS_TO_BE_AT_LEAST_A_WEEK();
     /// @notice Admin NFT recovery timelock max is 1 year
     error RECOVER_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_YEAR();
+    /// @notice Admin NFT recovery timelock max is 1 year
+    error RECOVER_TIMELOCK_SHOULD_BE_LATER();
     /// @notice The given user has not won
     error USER_HAS_NOT_WON();
     /// @notice Cannot re-draw yet

2. Let lastResortTimelockOwnerClaimNFT() execute after drawTimelock:

diff --git a/src/VRFNFTRandomDraw.sol b/src/VRFNFTRandomDraw.sol
index 668bc56..170ab02 100644
--- a/src/VRFNFTRandomDraw.sol
+++ b/src/VRFNFTRandomDraw.sol
@@ -307,6 +307,9 @@ contract VRFNFTRandomDraw is
             // Stop the withdraw
             revert RECOVERY_IS_NOT_YET_POSSIBLE();
         }
+        if (request.drawTimelock > block.timestamp) {
+            revert RECOVER_SHOULD_AFTER_DRAW_TIMELOCK();
+        }

         // Send event for indexing that the owner reclaimed the NFT
         emit OwnerReclaimedNFT(owner());
diff --git a/src/interfaces/IVRFNFTRandomDraw.sol b/src/interfaces/IVRFNFTRandomDraw.sol
index 4775288..2aac8a2 100644
--- a/src/interfaces/IVRFNFTRandomDraw.sol
+++ b/src/interfaces/IVRFNFTRandomDraw.sol
@@ -38,6 +38,7 @@ interface IVRFNFTRandomDraw {
     error DOES_NOT_OWN_NFT();
     /// @notice Too many / few random words are sent back from chainlink
     error WRONG_LENGTH_FOR_RANDOM_WORDS();
+    error RECOVER_SHOULD_AFTER_DRAW_TIMELOCK();

     /// @notice When the draw is initialized
     event InitializedDraw(address indexed sender, Settings settings);