Forgeries contest - Zarf's results

Lines of code

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

Vulnerability details

In order to guarantee the raffle NFT will not be stuck in the contract, there’s a last resort option which allows the admin the reclaim the NFT from the contract. However, the timestamp as from which this action can take place, is calculated based on when the contract was initialized.

In case an contract is initialized but is not directly used to start the contest, the owner might claim the NFT directly either before the draw even has started or during the draw before the winner is able to claim the NFT.

Impact

While users might think the draw process is honest, the owner could actually use a contract which was initialized recoverTimelock ago. Starting from this timestamp, the owner is always able to claim the NFT, regardless of the state of the contract.

Proof of Concept

1. Owner creates a new VRFNFTRandomDraw contract and chooses a recoverTimelock variable of 1 week (minimum amount is 1 week).
2. The owner waits at least 1 week before starting the contest (calling startDraw())
3. Let’s assume the owner changes in the meanwhile (this is not necessary for the attack but it shows the new owner is not necessarily the user who transfered the raffle tokenId to the contract.
4. The new owner is now able to rugpull the NFT whenever he/she wants.

Tool Used

Manual Review

Recommended Mitigation Steps

It’s recommended to reset the block timestamp upon which the admin can call the lastResortTimelockOwnerClaimNFT() function (block.timestamp + recoverTimelock) in each _requestRoll().

1. redraw() can be called before startDraw().

   Whenever startDraw() has not yet been called, redraw() could be called to initiate the first draw. However, this bypasses the SetupDraw event emit, which might confuse users.

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

2. It’s recommended to use Solidity time units (minutes, hours, days) to improve readability.

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

3. redraw() doesn’t emit an event.

   redraw() is a fairly important functionality which benefits from an emit event similarly to SetupDraw in startDraw()

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

4. Missing verification in fulfillRandomWords() to check whether the winning tokenId has an owner.

   In case the winning tokenId of the drawingToken has no owner (e.g. is not yet minted/doesn’t exist), the contract has to wait for drawBufferTime before it can be rerolled. If the winning tokenId has no owner, we can redraw directly and the contract is not ‘stuck’ for a determined time.

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

5. The contract does not support the approvers of the winning tokenId to withdraw the raffle tokenId.

   In some cases tokens are managed by the approvers (e.g. particular wallets/contracts). In case this particular NFT wins, the approvers will not be able to withdraw the raffle token id as winnerClaimNFT() checks whether msg.sender is the owner of the NFT.

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

6. The initializer of VRFNFTRandomDrawFactory does not call __UUPSUpgradeable_init()

   https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDrawFactory.sol#L31-L34