Forgeries contest - obront's results

Lines of code

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

Vulnerability details

Impact

The owner of a draw contract is able to call the lastResortTimelockOwnerClaimNFT() to reclaim their NFT, as long as the recoverTimelock has passed.

However, because the owner has complete control over when the startDraw() function is called, they are able to wait for the recovertTimelock without actually following through with the contest.

The result is that the timelock is essentially useless, as owners are able to withdraw the NFT without ever honoring the contest offer.

It's not hard to imagine a situation where a contest was promoted and users may have invested in the drawingToken in anticipation of the contest, and ended up harmed by these actions.

In an extreme case, the team behind the drawingToken could maliciously create raffles to get users to invest in their NFT and not follow through with them.

Proof of Concept

• recoverTimelock is set as a variable in settings
• startDraw() needs to be called by the owner, and if it isn't called, nothing happens
• lastResortTimelockOwnerClaimNFT() only looks at whether recoverTimelock has passed before allowing the owner to unlock the NFT

Tools Used

Manual Review

Recommended Mitigation Steps

There are two ways to approach mitigating this issue:

1. The simpler is that recoverTimelock is changed to a variable that counts the amount of time after startDraw() is called that the unlock can happen. The startDraw() call time would need to be recorded in state, and the check in lastResortTimelockOwnerClaimNFT() adjusted to account for this.

2. The better representation would be to ensure that owners follow through with the contests that they laid out. This would probably mean pre-committing to a number of redraws they intend to do, and keeping a counter. Then the lastResortTimelockOwnerClaimNFT() could check this counter, as well as the recoverTimelock to ensure that the owner has both waited sufficiently and followed through on their commitment.

[L-1] InitializedDraw event emits wrong parameters

In VRFNFTRandomDraw.sol#initialize(), we emit the following event:

emit InitializedDraw(msg.sender, settings);

However, this function is called by the Factory, so msg.sender will always be the Factory address.

Recommended Mitigation

The original msg.sender is passed to this function as admin, so the event should be replaced with:

emit InitializedDraw(admin, settings);

[L-2] redraw() can be called for first draw

There is no check in redraw() that it is actually a redraw. The only different check is that request.drawTimelock >= block.timestamp, but when we are starting the first draw, request.drawTimelock == 0, which will pass this check.

Later in the function, we check that the contract already holds the NFT, but it would be easy for any user to transfer it in manually before calling redraw().

This can't be used to cause any harm to the protocol, but we should be enforcing that users use the startDraw() function for their first draw.

Recommended Mitigation

Add the following check at the top of redraw():

if (request.drawTimelock == 0) {
    revert CANNOT_REDRAW_BEFORE_DRAW();
}

[QA-1] Unnecessary check on drawTimelock in _requestRoll()

In _requestRoll() we perform the following check:

if (
    request.hasChosenRandomNumber &&
    request.drawTimelock != 0 &&
    request.drawTimelock > block.timestamp
) 

The request.drawTimelock != 0 check is intended to check whether it's the first draw. However, in the situation where request.drawTimelock == 0, it will also be less than block.timestamp, so the additional check is unnecessary.

Recommended Mitigation

Replace with:

if (
    request.hasChosenRandomNumber &&
    request.drawTimelock > block.timestamp
) 

[QA-2] Unnecessary check of request ID in startDraw()

When startDraw() is called, the first thing we do is the following check:

if (request.currentChainlinkRequestId != 0) {
    revert REQUEST_IN_FLIGHT();
}

However, the next action we take is to call _requestRoll(), where this exact check is immediately done. There is no need to do it twice.

Recommended Mitigation

Remove the check from the top of startDraw().