Forgeries contest - SmartSek's results

Lines of code

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

Vulnerability details

Impact

A fraudulent raffle can be ran by admin because recoverTimelock is set in the initialize function. This enables an admin to withdraw the prize before the rightful winner can claim it.

Proof of Concept

• admin sets recoverTimelock for just over 1 week after initialize is called.
• after recoverTimelock time has been reached admin calls startDraw.
• Before winner can withdraw their NFT admin calls lastResortTimelockOwnerClaimNFT and effectively cheats the contest timelock ssytem.

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

        if (_settings.recoverTimelock < block.timestamp + WEEK_IN_SECONDS) {
            revert RECOVER_TIMELOCK_NEEDS_TO_BE_AT_LEAST_A_WEEK();
        }

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

    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
        );
    }

Recommended Mitigation Steps

Set recoverTimelock during startDraw() or preferably as mentioned in another finding, call startDraw() during initialize().

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L125-L136 https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L172-L198

Vulnerability details

Impact

admin can sell, transfer or have their tokenId NFT stolen via phishing attack to another address after initialising the VRFNFTRandomDraw contract. This would make it impossible to startDraw because admin is no longer in possession of tokenId.

An admin with the intention of never really going through with the raffle could maliciously initialize a raffle just for PR purposes and then back out.

This makes the contract far from trustless.

Proof of Concept

initialize() checks if admin owns NFT but startDraw() doesn't.

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

        // Get owner of raffled tokenId and ensure the current owner is the admin
        try
            IERC721EnumerableUpgradeable(_settings.token).ownerOf(
                _settings.tokenId
            )
        returns (address nftOwner) {
            // Check if address is the admin address
            if (nftOwner != admin) {
                revert DOES_NOT_OWN_NFT();
            }
        } catch {
            revert TOKEN_BEING_OFFERED_NEEDS_TO_EXIST();

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

    function startDraw() external onlyOwner returns (uint256) {
        // Only can be called on first drawing
        if (request.currentChainlinkRequestId != 0) {
            revert REQUEST_IN_FLIGHT();
        }

        // Emit setup draw user event
        emit SetupDraw(msg.sender, settings);

        // Request initial roll
        _requestRoll();

        // Attempt to transfer token into this address
        try
            IERC721EnumerableUpgradeable(settings.token).transferFrom(
                msg.sender,
                address(this),
                settings.tokenId
            )
        {} catch {
            revert TOKEN_NEEDS_TO_BE_APPROVED_TO_CONTRACT();
        }

        // Return the current chainlink request id
        return request.currentChainlinkRequestId;
    }

For the reasons above and others mentioned in separate findings, I believe the disjointed execution of initialize() and startDraw() allows for admin to manipulate incentives for users to join a raffle and not follow through with his promise.

Recommended Mitigation Steps

Make startDraw() internal and call it within initialize().