Forgeries contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

Because recoverTimelock can be less than block.timestamp + drawBufferTime when startDraw is called, it's possible that user will win nft, but owner will call lastResortTimelockOwnerClaimNFT before winner will call winnerClaimNFT.

Proof of Concept

When VRFNFTRandomDraw contract is initialized it is provided with few variables. We will consider drawBufferTime and recoverTimelock. drawBufferTime time is needed to give winner some time to claim nft. It should be in range [1hour..1month]. Once new request for random is sent then request.drawTimelock = block.timestamp + drawBufferTime. recoverTimelock is the timestamp when owner can withdraw nft from contract. It is designed to be used if noone claimed nft for long time. It should be at least one week in future. When this data has passed then user can call lastResortTimelockOwnerClaimNFT function.

Owner can manipulate with lottery in following way.

Scenario. 1.Owner created contract and initialized with recoverTimelock in 1 week and drawBufferTime 1 day. 2.Owner didn't call startDraw for 7 days. 3.On 8th day when recoverTimelock has passed owner calls startDraw function. 4.The winner is found to be Alice, but owner doesn't like Alice and he calls lastResortTimelockOwnerClaimNFT function and withdraw nft. 5.As result Alice has won, but couldn't claim nft. Owner can in such way filter winners.

Tools Used

VsCode

Recommended Mitigation Steps

Inside lastResortTimelockOwnerClaimNFT use such check. This will leave winners time to claim nft.

    if (settings.recoverTimelock > block.timestamp || block.timestamp < request.drawTimelock) {
            // Stop the withdraw
            revert RECOVERY_IS_NOT_YET_POSSIBLE();
        }

Lines of code

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

Vulnerability details

Impact

After lastResortTimelockOwnerClaimNFT is called function hasUserWon will still show that user is winner.

Proof of Concept

When settings.recoverTimelock has passed owner can withdraw nft using lastResortTimelockOwnerClaimNFT function. https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L304-L320

    function lastResortTimelockOwnerClaimNFT() external onlyOwner {
        // If recoverTimelock is not setup, or if not yet occurred
        if (settings.recoverTimelock > block.timestamp) {
            // Stop the withdraw
            revert RECOVERY_IS_NOT_YET_POSSIBLE();
        }


        // Send event for indexing that the owner reclaimed the NFT
        emit OwnerReclaimedNFT(owner());


        // Transfer token to the admin/owner.
        IERC721EnumerableUpgradeable(settings.token).transferFrom(
            address(this),
            owner(),
            settings.tokenId
        );
    }

Note, that this function doesn't do anything with request variable. In case if before lastResortTimelockOwnerClaimNFT function was called winner was choosed then request variable is provided with winner data.

And then when winner will call hasUserWon function it will still show that user won, but he is not able to claim. This is because request.hasChosenRandomNumber is still true. https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L264-L274

    function hasUserWon(address user) public view returns (bool) {
        if (!request.hasChosenRandomNumber) {
            revert NEEDS_TO_HAVE_CHOSEN_A_NUMBER();
        }


        return
            user ==
            IERC721EnumerableUpgradeable(settings.drawingToken).ownerOf(
                request.currentChosenTokenId
            );
    }

As result user has shown incorrect info, as he was winner in the past, but in present moment he is not winner as winner can claim nft, but nft is already transferred to owner.

Tools Used

VsCode

Recommended Mitigation Steps

When lastResortTimelockOwnerClaimNFT is called set request.hasChosenRandomNumber to false.

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L204-L206

Vulnerability details

Impact

In case if request was sent, but result was not received it should not be possible to call redraw function

Proof of Concept

redraw function is created to ask for new random form chainlink in case if user hasn't claimed nft. https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L204-L206

        if (request.drawTimelock >= block.timestamp) {
            revert TOO_SOON_TO_REDRAW();
        }

This check is present to control when you can call redraw. In case if respond time from chainlink is more than request.drawTimelock, then there is no need to call redraw as it's just waste of gas. You just need to wait, maybe chainlink subscription is not funded.

Tools Used

VsCode

Recommended Mitigation Steps

Use such check.

        if (request.drawTimelock >= block.timestamp || !request.hasChosenRandomNumber) {
            revert TOO_SOON_TO_REDRAW();
        }