Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. A compromised owner of VRFNFTRandomDraw can claim the NFT to another accomplice addresss

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. A compromised owner of VRFNFTRandomDraw can claim the NFT to another accomplice address as follows: 1) call transferOwnership(accomplice) to transfer the ownership to accomplice 2) use account of accomplice to call lastResortTimelockOwnerClaimNFT() to claim the NFT.

Tools Used

Remix

Recommended Mitigation Steps

The fix is easy: just record the original owner of the NFT, and enforce that the NFT can be retrieved only to the original owner address. See below

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

        originalOwner = msg.sender; // @record the original owner

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

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),
            originalOwner,           // @only retrieved to the original owner
            settings.tokenId
        );
    }

code-423n4 / 2022-12-forgeries-findings