Forgeries contest - imare's results

Lines of code

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

Vulnerability details

lastResortTimelockOwnerClaimNFT() as the name says is used in case the winning user doesn't retrieve the won NFT token and in such case the owner can rescue the NFT from the contract.

The mentioned function can be only called after a certain period is passed:

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

This and the onlyOwner are the only check that prevents the function from being executed.

The problem is that the variable settings.recoverTimelock is only checked to be correct when initializing the contract. But the beginning of the draw can happen at a much later time.

The minimal time checked for recoverTimelock is set to be a least a week.

If a user/owner creates a draw and waits a week plus one second to call startDraw() now even if the draw is not yet finished the owner can immediately call lastResortTimelockOwnerClaimNFT() to regain the NFT back.

Impact

When the winning user is chosen it can no longer get the NFT because is already gone. There was no waiting time for the user to retrieve the NFT token even if the draw just started.

Proof of Concept

The only place that checks settings.recoverTimelock is in the initialization routine:

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

Tools Used

Manual review

Recommended Mitigation Steps

Use settings.recoverTimelock variable like is done with settings.drawBufferTime.

In the initialization method check if the interval is correct.

Then only in _requestRoll() recalculate the real end for recoverTimelock.

In the meantime the user/owner will not be able to call lastResortTimelockOwnerClaimNFT() because the if check will still fail with the recoverTimelock value being smaller then block.timestamp.

// this will still fail and will protect the function from being called before startDraw() + recoverTimelock time 
if (settings.recoverTimelock > block.timestamp) { 

Note: the recalculation of recoverTimelock can also be done once in startDraw() where the NFT is first transferred (and locked) to the contract. But in this case if a redraw happens the owner again can get the NFT out much faster then the winning user.