Owner can rug reward NFT from the winner

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L83-L88
https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L159
https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L314-L317

Vulnerability details

Impact

Settings variable drawBufferTime represents the buffer time that forbids owner to trigger redraw and thus allows winner to claim the reward NFT via winnerClaimNFT function. There is also another recoverTimelock settings variable that defines time when the owner can reclaim the reward NFT via lastResortTimelockOwnerClaimNFT. The issue is that drawBufferTime and recoverTimelock are not correctly checked which leads to situation that both triggering winnerClaimNFT and lastResortTimelockOwnerClaimNFT is possible.

Example 1:

1. Draw is initialized with:
   • drawBufferTime 1 month
   • recoverTimeLock 1 week
2. startDraw() is called by the owner
   • drawTimeLock is set to the next month
   • recoverTimeLock is always set to 1 week
3. Winner thinks that he has 1 month to claim the reward NFT, but after 1 week owner withdraws NFT not letting the winner to claim the reward.

The issue is also present in case there are multiple redraws (via redraw function) that lead to expiration of recoverTimeLock which allows owner to trigger lastResortTimelockOwnerClaimNFT function.

Proof of Concept

• https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L83-L88
• https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L159
• https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L314-L317

Recommended Mitigation Steps

It is recommended to set drawBufferTime and recoverTimeLock to values that will hold relationship recoverTimeLock = block.timestamp + drawBufferTime + X. In addition value of recoverTimeLock should be set properly in every _requestRoll function execution. Disallow owner to withdraw the NFT before the drawTimelock is expired.

[c4-judge]

gzeon-c4 marked the issue as duplicate of #146

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)