Forgeries contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

owner can retrieve the token although drawing is in progress

Proof of Concept

lastResortTimelockOwnerClaimNFT() Be used in: " If no users ultimately claim the NFT, the admin specifies a timelock period after which they can retrieve the raffled NFT. "

But it is no check that if draw is already in progress, i.e. the current round of request.drawTimelock >= block.timestamp it should not be able to be lastResortTimelockOwnerClaimNFT(). Because a new round of draw has already started and the results may be available.

The above problems can occur in the following two cases： 1: Because: _settings.drawBufferTime >= HOUR_IN_SECONDS and <= MONTH_IN_SECONDS _settings.recoverTimelock > = WEEK_IN_SECONDS and <= MONTH_IN_SECONDS * 12

if drawBufferTime = two weeks and recoverTimelock = one weeks

So: this condition is possible: drawBufferTime > recoverTimelock

2: The first few rounds have been unfulfilled and keep repeating to start a new round, resulting in request.drawTimelock > settings.recoverTimelock will appear

Tools Used

Recommended Mitigation Steps

add check 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 STILL_IN_DRAWING();
+       }
        ...