code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Attackers can create a fake raffle with an NFT that is not owned and cheat users.

Proof of Concept

New raffles are created by the function makeNewDraw() of the VRFNFTRandomDrawFactory.sol.

VRFNFTRandomDrawFactory.sol
38:     function makeNewDraw(IVRFNFTRandomDraw.Settings memory settings)
39:         external
40:         returns (address)
41:     {
42:         address admin = msg.sender;
43:         // Clone the contract
44:         address newDrawing = ClonesUpgradeable.clone(implementation);
45:         // Setup the new drawing
46:         IVRFNFTRandomDraw(newDrawing).initialize(admin, settings);
47:         // Emit event for indexing
48:         emit SetupNewDrawing(admin, newDrawing);
49:         // Return address for integration or testing
50:         return newDrawing;
51:     }

And whenever a new raffle is created, msg.sender is used for initialization and it is checked if the caller is actually the owner of the token. From this, I believe that the protocol intended to guarantee that the token owner actually created a raffle, i.e. the existence of a legitimate raffle is supposed to mean that the actual token owner is going to draw and throw his NFT as a prize.

VRFNFTRandomDraw.sol
75:     function initialize(address admin, Settings memory _settings)
76:         public
77:         initializer
78:     {
...
124:
125:         // Get owner of raffled tokenId and ensure the current owner is the admin
126:         try
127:             IERC721EnumerableUpgradeable(_settings.token).ownerOf(
128:                 _settings.tokenId
129:             )
130:         returns (address nftOwner) {
131:             // Check if address is the admin address
132:             if (nftOwner != admin) {
133:                 revert DOES_NOT_OWN_NFT();
134:             }
135:         } catch {
136:             revert TOKEN_BEING_OFFERED_NEEDS_TO_EXIST();
137:         }
138:     }

The problem is that the factory does not track the raffles that were created from it. Because the implementation is exposed public, attackers can clone it and initialize it with malicious parameters. Especially, the attacker can call the initialize() function with NFT that he does not own. He just needs to use the actual token address and real owner address of the NFT. For example, Bored Ape Yacht Club #6441 can be used with initialize(0xcc09c1be4dd06E3cf5B76f27742168956EB17f08, Settings(token=0xBC4CA0EdA7647A8aB7C2061c2E118A18a936f13D, tokenId=6441,...)). Of course the attacker can not own this raffle and the actual token owner will be the owner of this raffle. But the point is that it is enough to create hundreds of fake raffles bypassing the requirements of token ownership. And this breaks the guarantees that the actual token owner has created this raffle.

Tools Used

Manual Review

Recommended Mitigation Steps

Make raffles to be verifiable, maybe you can maintain a mapping of all legitimate raffles (that are created by makeNewDraw()) in the factory contract.

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)