search

Cyfrin / foundry-smart-contract-lottery-cu

48 stars 42 forks source link

Potential issue in fulfillRandomWords in Raffle.sol #39

Closed milosdjurica closed 11 months ago

milosdjurica commented 11 months ago

In src/Raffle.sol -> https://github.com/Cyfrin/foundry-smart-contract-lottery-f23/blob/main/src/Raffle.sol in method called fulfillRandomWords() winner is picked and money is sent to him.

The potential issue is on the line 176 s_raffleState = RaffleState.OPEN; We set state of the Raffle to be open, even before money is sent to the winner. This could potentially lead to new user entering the lottery before money is sent to the winner. Then the winner would get all the money from that lottery cycle and additionally he would get money from the ticket of the user who was supposed to enter new lottery cycle.

Moving line 176 s_raffleState = RaffleState.OPEN; under the line 179 (bool success, ) = recentWinner.call{value: address(this).balance}(""); should fix this issue.

I am no expert in Solidity, I just started learning it with this course, but I have been coding for years, and I thought that this might cause an issue. Please let me know if I am wrong about this.

PatrickAlphaC commented 11 months ago

Not quite! So the blockchain is considered synchronous, which means that the entire function of fulfillRandomWords has to complete before someone else can do anything.

So whether or not we put that line earlier or later in the function doesn't matter, since someone can only call a different function after that entire function call has completed!

(well... that's not the whole story, there is an issue of potential reentrancy happening with the call function... but you can ignore that for now!)

milosdjurica commented 11 months ago

Thanks a lot, that makes a lot of sense! Learning something new every day :)