First Flight #2: Puppy Raffle

Summary

withdrawFees() has a wrong check that can prevent fees to be withdrawn to the protocol owners.

Vulnerability Details

This check is not adequate and I can see 2 ways to exploit it:
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

1. An attacker can donate any amounts to the contract and have withdrawFees() forever bricked.

2. If the fees are not immediately collected after a winner is selected and someone entered the raffle, then the equality in the check will be broken, because totalFees is only updated when selecting a winner. This can be a problem, as an attacker can monitor the mempool for withdrawFees() and keep entering the raffle and refunding himself.

Impact

Fees funds will be forever lost in the contract.

Tools Used

Manual Review.

Recommendations

Instead of using address(this).balance to check for active players, they can use players.length. It is 0 when a winner is selected and there are no more active players. Actually, this check is not even needed, they can withdraw the fees even when players are active as accounting for fees is separate from the winner's mount and is only set when selecting a winner. As such there is no risk of fees being incorrect due to new participant in each raffles or refunds. This would also mitigate (2).