Forgeries contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

Raffle creators can start a draw at close to recoverTimelock intentionally and frontrun winnerClaimNFT() to prevent winners from claiming the NFT

Proof of Concept

Creators can start new raffles by providing their own settings and there exists a field recoverTimelock that is intended to prevent creators from withdrawing an NFT before winners. I believe this is to prevent the raffle owners withdraw the prize NFT before the winner claims.

On initialization, there is a check requiring this recoverTimelock to be at least a week from the creation time. But there is no other restrictions on the raffle owner when to start draw. So the owner can start a draw right before that recoverTimelock or even after that time.

A clever owner might estimate the waiting time from the requestRandomWords() till when the callback fulfillRandomWords() is called. The owner can use that estimation to decide when to start a draw. (The waiting time can (vary)[https://ethereum.stackexchange.com/questions/134781/what-is-the-recommended-waiting-time-between-requestrandomwords-and-fulfillrando])

The owner still can trick users that he is doing a fair raffle.

But once a callback is called and a winner is decided, the owner can check the winner and act selectively according to the winner. If he does not want the winner take the prize, he can easily frontrun the winner's call to winnerClaimNFT() and call lastResortTimelockOwnerClaimNFT() to get the NFT back. The winner's call will revert because the NFT is not in the raffle contract anymore.

Of course, I understand that the settings are public to users and it might be hard to trick users. But I believe this is a possible scenario and the protocol can be abused by malicious creators.

VRFNFTRandomDraw.sol
304:     function lastResortTimelockOwnerClaimNFT() external onlyOwner {
305:         // If recoverTimelock is not setup, or if not yet occurred
306:         if (settings.recoverTimelock > block.timestamp) {
307:             // Stop the withdraw
308:             revert RECOVERY_IS_NOT_YET_POSSIBLE();
309:         }
310:
311:         // Send event for indexing that the owner reclaimed the NFT
312:         emit OwnerReclaimedNFT(owner());
313:
314:         // Transfer token to the admin/owner.
315:         IERC721EnumerableUpgradeable(settings.token).transferFrom(
316:             address(this),
317:             owner(),
318:             settings.tokenId
319:         );
320:     }

Tools Used

Manual Review

Recommended Mitigation Steps

• Consider limiting a creator to start a draw after some deadline. (like an hour from the recoverTimelock)
• Consider adding another time lock dedicated for winners to claim NFT. Or it is also possible to store the time when the winner was chosen and disallow owner to retrieve NFT for some time from then.

Lines of code

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

Vulnerability details

Impact

It is not guaranteed that the raffle owner is the actual owner of the token up for a raffle.

Proof of Concept

And whenever a new raffle is created, msg.sender is used for initialization and it is checked if the caller is actually the owner of the token. From this, I believe that the protocol intended to guarantee that the raffle owner actually owns the token up for a raffle.

VRFNFTRandomDraw.sol
75:     function initialize(address admin, Settings memory _settings)
76:         public
77:         initializer
78:     {
...
124:
125:         // Get owner of raffled tokenId and ensure the current owner is the admin
126:         try
127:             IERC721EnumerableUpgradeable(_settings.token).ownerOf(
128:                 _settings.tokenId
129:             )
130:         returns (address nftOwner) {
131:             // Check if address is the admin address
132:             if (nftOwner != admin) {
133:                 revert DOES_NOT_OWN_NFT();
134:             }
135:         } catch {
136:             revert TOKEN_BEING_OFFERED_NEEDS_TO_EXIST();
137:         }
138:     }

But this guarantee can be easily broken by transferring the ownership of the raffle. This means a user can create unlimited number of raffles for the same token and transfer ownership to others. Although the other owners can not start a draw, the impact might differ according to the business logic in the sense of how the ownership of a raffle for a token is valued. I believe this is not what the protocol intended.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider locking the NFT token during the initialization.