First Flight #2: Puppy Raffle

Summary

withdrawFees can only be called when the balance of the contract is equal to totalFees (which is a tally of the fees owed to the contract owner). There are several reasons this doesn't make sense. First, something could go wrong where the contract address' balance is not in line with totalFees (e.g., someone abusing refund to drain most or all of the contract) and having this check would lock funds in the contract if for some reason these two figures were not in sync. It could also get out of sync due to rounding issues. Another reason this doesn't make sense is that the error statement is "there are currently active players" (which I interpret to mean the lottery is currently open) but a better way to have that limitation is to have an enum for lottery state that is turned from OPEN to CLOSED rather than a backhanded way of trying to check if a lottery is ongoing that depends on these two figures being exactly equal. It is possible for there to be no active players, but the two figures aren't equal.

Vulnerability Details

Impact

The fees could be irretrievable for a period of time if the contract's balance is out of sync with totalFees.

Tools Used

Manual review

Recommendations

As I recommended in another finding, use an enum for lottery state OPEN and CLOSED and then, if you want to limit withdrawing of the fees while the lottery is closed, add a custom error PuppyRaffle__LotteryNotOpen() to prevent withdrawFees from being accessed while the lottery isn't open.