First Flight #2: Puppy Raffle

Summary

a smart contract address which wins the raffle may not be able to receive their winnings, and will forfeit their win and nft.

Vulnerability Details

If the winner is a smart contract that does not have a fallback or receive function it can not receive eth, therefore the selectWinner function will revert. however on the next call to selectWinner, a new winner will be chosen. this means the player is guaranteed to lose their entry fee, and whoever called selectWinner will have wasted gas.

Impact

It is relatively low impact, but can lead to some unexpected situations.
1)If there are very few players in the raffle(2-5), even just 1 of them is a smart contract which does not accept
it can take many calls to successfully select a winner, wasting gas.

2)There is also a scenario, where if a raffle happens to have all players being contracts with no fallback, the contract will be stuck permanently on that specific raffle, unable to ever select a winner.
this situation is possible in a raffle contract that does not get much interaction, it is not the most farfetched thing in the world, for a contract bootstrapping itself to have low amount of users interacting with it and where it may have very few entries in raffles.

3)A malicious actor may also try to heavily populate a raffle with contract addresses with no fallback, in an attempt to DOS the raffle, although it is most likely that there would be no reason to do this for the sake of profit, and may cost a prohibitive amount of eth.

Tools Used

Manual Review

Recommendations

One solution is to accept it as a known limitations and try to document and make as clear as possible that a contract address in the raffle must be able to receive ether.

One potential issue that could be fixed is to remove players who prove to not have callback functions, to ensure that their address will not be selected again as a winner.
this would require a rework from the way the ether is called.

you could have an event like :
event ForfeitedWin(address indexed winner);

and use the low level call which does not revert.
This solution has it's own complexities which need to be dealt with (reentrancy).

And in the selectWinner function instead of sendValue:
(bool success, ) = winner.call{value: address(this).balance}("");
if (!success) {
// Remove the 'winner' from the 'players' array.
players[index] = players[players.length - 1];
players.pop();

    emit ForfeitedWin(winner);
} else{...}