Forgeries contest - Apocalypto's results

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.

Lines of code

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

Vulnerability details

Impact

Immutable variable MONTH_IN_SECONDS is assigned with incorrect value. It is expected to represent 30 days but it is set to 30 weeks:

uint256 immutable MONTH_IN_SECONDS = (3600 * 24 * 7) * 30;

Proof of Concept

VRFNFTRandomDraw.sol:

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

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to assign correct value of 30 days to MONTH_IN_SECONDS variable:

uint256 immutable MONTH_IN_SECONDS = (3600 * 24) * 30;