Forgeries contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

The owner is able to select the winner, so it is not a fair draw. The owner starts the draw, then the random number is chosen. If the random number is not what the owner prefers, he can can cancel the draw. Then again the owner can execute redraw (after the drawTimelock and transferring the NFT to the contract VRFNFTRandomDraw ). The owner can apply this until the random number is what he prefers.

Proof of Concept

The following conditions should be met during initialization of a draw:

• 1 hour <= drawBufferTime <= 1 month
• the time that the draw is initialized + 1 week <= recoverTimelock <= the time that the draw is initialized + 1 year Moreover, when starting the draw, drawTimelock is set to time of requesting the roll + drawBufferTime.

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L83-L98

The way the recovery time is used can be unfair to the winner and gives the owner this ability to select the winner. Consider the following scenario:

• The owner initializes the draw with drawBufferTime = 10 days and recoverTimelock = 1 week.
• Then the owner after two weeks starts the draw by calling startDraw. So, drawTimelock = current time + 10 days https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L173
• After the random number is chosen, the winner should be able to claim the NFT by calling winnerClaimNFT. Since it is in the drawTimelock, the owner can not execute redraw. https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L277
• But the owner is able to cancel the draw at any time by calling lastResortTimelockOwnerClaimNFT. Because, this function only checks that the current timestamp is larger than recoverTimelock or not. Since, it is passed almost 2 weeks, and recoverTimelock = 1 week, the owner is able to cancel the draw at anytime.

if (settings.recoverTimelock > block.timestamp) {
            // Stop the withdraw
            revert RECOVERY_IS_NOT_YET_POSSIBLE();
        }

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

• Although, it is in the drawTimelock and the winner has the right to claim the NFT, but the owner can cancel it at any time, so the winner will not be able to claim the NFT. This is not fair, because the owner can cancel the draw when the winner is not what the owner prefers. In other words, the lottery will not be really random, and the owner has the ability to select the winner.
• Then in order to start the draw again, the owner transfers the NFT to contract VRFNFTRandomDraw, and then calls redraw. https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L203

In summary the owner cancels the draw if the winner is not what he prefers before the winner claims his NFT. This can be done by owner if the recoverTimelock is passed. This value can be easily passed by setting it to 1 week and after 1 week starting the draw.

Tools Used

Recommended Mitigation Steps

It is recommended to add a condition that owner can cancel the draw only after the drawTimelock .

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

       if (request.drawTimelock >= block.timestamp) {
            revert TOO_SOON_TO_CANCEL_DRAW();
        }

        // Send event for indexing that the owner reclaimed the NFT
        emit OwnerReclaimedNFT(owner());

        // Transfer token to the admin/owner.
        IERC721EnumerableUpgradeable(settings.token).transferFrom(
            address(this),
            owner(),
            settings.tokenId
        );
    }