Forgeries contest - 9svR6w's results

Lines of code

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

Vulnerability details

Impact

The contest readme states "This contract should prevent the owner from iterrupting the contest until the timelock unlocks at the end"

The recoverTimelock is set based on the time VRFNFTRandomDraw initialize() is called not based on the the time startDraw() or redraw() is called. A malicious owner could potentially:

1. create the raffle
2. wait until the recoverTimelock expires or is about to expire
3. call startDraw() to actually begin the raffle
4. immediatly call lastResortTimelockOwnerClaimNFT() to claim the reward NFT before the raffle winner can claim the reward. Potentially this call to lastResortTimelockOwnerClaimNFT() can occur within the same transaction as the above startDraw().

The above sequence of events prevents the raffle winner from claiming the reward, thus interrupting the normal course of the contest.

In fact, a malicious raffle creator/owner could implement such a contest without ever owning the reward NFT outright. A flashloan of the NFT could be taken at the time the contest is created, and then again for startDraw() and lastResortTimelockOwnerClaimNFT() if these two are combined in a single transaction.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Set recoverTimelock to a value in the future whenever startDraw() or redraw() is called. Alternatively, require that startDraw() / redraw() be called within a certain window before recoverTimelock expires.

Lines of code

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

Vulnerability details

Impact

The admin/owner of VRFNFTRandomDraw can startDraw() a raffle, including emitting the SetupDraw event, but in a way that ensures fulfillRandomWords() is never called. For example:

• keyHash is not validated within coordinator.requestRandomWords(). Providing an invalid keyHash will allow the raffle to start but prevent the oracle from actually supplying a random value to determine the raffle result.
  • https://github.com/smartcontractkit/chainlink/blob/00f9c6e41f843f96108cdaa118a6ca740b11df35/contracts/src/v0.8/VRFCoordinatorV2.sol#L407-L409
  • https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L163
• The admin/owner could alternatively ensure that the owner-provided chain.link VRF subscription does not have sufficient funds to pay at the time the oracle attempts to supply random values in fulfillRandomWords().
  • https://github.com/smartcontractkit/chainlink/blob/00f9c6e41f843f96108cdaa118a6ca740b11df35/contracts/src/v0.8/VRFCoordinatorV2.sol#L594-L596

In addition, the owner/admin could simply avoid ever calling startDraw() in the first place.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Depending on the desired functionality with respect to the raffle owner, a successful callback to fulfillRandomWords() could be a precondition of the admin/owner reclaiming the reward NFT. This would help ensure the owner does not create raffles that they intend will never pay out a reward.

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

• This is actually 30 weeks, not 30 days. Validation related to MONTH_IN_SECONDS will be affected.

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

• In the current code, _requestRoll() will never be called in cases where any if these if conditionals will be true; the reverts cannot be triggered

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

• Incorrect comment, this is not necessarily the first time requestRandomWords() is called

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

• Recommend that redraw() emit an event like startDraw() does so users will be aware the raffle is being re-run.

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

• typo in comment

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

• recommend using IERC721 safeTransferFrom()

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

• recommend using IERC721 safeTransferFrom()

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L127 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L187 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L216 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L271 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L295 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L315

• Can perform these calls using simply IERC721 interface rather than IERC721EnumerableUpgradeable