`admin` can flashloan NFT to fake ownership.

[code423n4]

Lines of code

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

Vulnerability details

Impact

admin could initialize() a draw by flash loaning an NFT without ever really owning it and having no intention to go through with the raffle.

Proof of Concept

No checks to ensure admin is the legitimate owner of the NFT and has the power to permanently transfer it to another address.

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

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

Recommended Mitigation Steps

Transfer NFT to contract during initialize() so admin can't fake ownership.

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as primary issue

[gzeoneth]

I think it make sense to lock the prize in the contract before calling startDraw.

[trust1995]

As long as startDraw and redraw are permissioned, it seems like a design choice that owner doesn't have to give away the NFT. They can always take it back by waiting until recovery time. Therefore, decision on time to transfer the NFT seems to be more of a preference than a MED level impact.

[gzeoneth]

Generally agree what you said, but the factory attempted to check it so I think it also make sense to point out there is a bypass. Btw this is presort the severity is not final.

[iainnash]

I would argue that this is a low level issue if any, because when the raffle is started, it escrows the NFT which would fail at that point. The contract existing in my opinion is not a valid case to assume the user won't rug the NFT but only when the raffle is initiated.

The better way to do this would be to remove startDraw and initialize the drawing when the contract is created.

[hansfriese]

In my opinion, there is a valid point in this issue. The protocol checks the ownership of the NFT during the initialization and I believe it intends to prove something to the users even before a draw. It depends on the business logic, especially how the existence of a raffle is valued. From my viewpoint, I think it is reasonable to lock NFT on the initialization to prevent multiple instances of the raffle being created for the same NFT.

[iainnash]

@hansfriese I think the main question here is does initialization mean that the raffle starts. The initialization of the contract as designed i to initialize the contract and startDraw kicks off the draw. The escrow comes only after the draw is started not when the contract is created. In the UI and design of the contract the naming is pretty clear that the draw isn't started until startDraw is called.

[gzeoneth]

I am keeping this as valid since the factory attempted to check the ownership of the NFT, which if its not expected to be escrowed immediately will make the check moot.

[iainnash]

That’s fair. I’ll mark it as confirmed once I’m back on my computer.

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)

[c4-judge]

gzeon-c4 changed the severity to 2 (Med Risk)

[C4-Staff]

captainmangoC4 marked issue #88 as primary and marked this issue as a duplicate of 88