Forgeries contest - indijanc's results

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L304

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.

Wrap calculations in unchecked block if guaranteed not to over- or underflow

VRFNFTRandomDraw.sol L112-L117 If statement with subtraction can be wrapped in unchecked because the statement already checks the first parameter shouldn't be smaller than the second. This slightly reduces contract size and function gas cost. Tests show the following gas cost reduction:

All event emitting can be moved to end of functions

The event emitting can be moved to the end of the functions. This save some gas in error cases and is in line with the checks-effects-interactions paradigm. There's some places where event emitting is done before checks.

Redundant check in startDraw()

VRFNFTRandomDraw.sol L175-L177 The check for currentChainlinkRequestId in startDraw() is redundant because the check is already present in _requestRoll(). Remove the check from startDraw() function and also move emit SetupDraw(msg.sender, settings); after _requestRoll(); to save gas. Here's the benefits with these changes as observed from project tests:

Storage variable can be cached in local variable

VRFNFTRandomDraw.sol L250 VRFNFTRandomDraw.sol L256 The storage variable settings.drawingTokenStartId is accessed twice inside fulfillRandomWords(). It can instead be cached in a local variable to save deployment and function gas cost. These are gas savings as observed with projects tests:

Redundant variable can be removed

VRFNFTRandomDraw.sol L279 The user local variable can be removed and msg.sender from calldata used instead. This slightly reduces contract size and function gas cost. Here's the project test results after removing the variable:

Another redundant variable can be removed in makeNewDraw()

VRFNFTRandomDrawFactory.sol L42 The admin local variable can be removed and msg.sender used instead to save some gas. Here's the savings:

Use msg.sender instead of owner() function calls

VRFNFTRandomDraw.solL312 VRFNFTRandomDraw.sol L317 lastResortTimelockOwnerClaimNFT() already ensures that msg.sender is the owner with the onlyOwner modifier. Using the owner() function calls instead of using msg.sender is redundant in this case and increases gas cost. Here's the benefits after using msg.sender instead, as seen with the project tests:

emitting events in OwnableUpgradeable can use msg.sender

OwnableUpgradeable.sol L91 OwnableUpgradeable.sol L109 OwnableUpgradeable.sol L129 Several events inside OwnableUpgradeable can use msg.sender as all calls are protected with onlyOwner modifier. This saves gas in several function calls: