search

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

0 stars 0 forks source link

Frontrunning the `winnerClaimNFT` is possible #334

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The winner in the NFT raffle may be frontrun if he has listed his NFT for sale on a marketplace, stealing his raffle NFT.

Proof of Concept

A new VRFNFTRandomDraw Clone contract is deployed using makeNewDraw, with some NFT collection as drawingToken and another as token. Let's say the token is more valuable than some of the drawingToken NFTs. The owner calls startDraw to start the raffle draw and the winner is owner of, for example, drawingToken id = 45. The winner has listed his NFT for sale on OpenSea. Frontrun scenario: Malicious attacker deploys a contract, which in one transaction buys the NFT from the winner on OpenSea, calls winnerClaimNFT, and optionally sells the bought NFT back, profiting a free raffle NFT.

Tools Used

VS Code, Forge

Recommended Mitigation Steps

Store the current raffle winner in a storage variable raffleWinner in VRFNFTRandomDraw.sol, on calling startDraw or redraw. Check against that variable on call to hasUserWon(user).

c4-judge commented 1 year ago

gzeon-c4 marked the issue as primary issue

gzeoneth commented 1 year ago

tbh I don't think this is an issue but wait for sponsor review

321 do suggested to use push instead of pull which kind of make sense if you consider this as an issue, I can imagine its a pita to delist all nft on opensea when there is a raffle going on

iainnash commented 1 year ago

I would argue this is not an issue.

If we were to store the raffle winner upon response from chainlink

  1. there would be more gas usage in the chainlink context.
  2. we'd see a report here around the lines of "users transferring NFTs after raffle results incorrectly buy a worthless NFT".

This to me feels that the winner of the owner of the NFT and the NFT can be freely traded until the claim is processed for the NFT – if you have the NFT up for sale that's the decision of the user. Since we use 3rd party contracts here there isn't a way to prevent trading – it could be integrated to the drawing contract to freeze trading when the raffle results have come back.

@emrecolako can weigh in

emrecolako commented 1 year ago

Agreed with @iainnash, it's 100% user's decision to put the NFT up for sale. We cannot prevent that from happening if they want to.

c4-sponsor commented 1 year ago

iainnash marked the issue as sponsor disputed

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid