Root + Impact

Description

PuppyRaffle.withdrawFees() in src/PuppyRaffle.sol will only pay out the fees if the contract balance is exactly equal to the recorded totalFees. If the contract holds even one wei more than that, the withdrawal reverts, and there is no other way to take the fees out, so they are locked forever.

The vulnerable code is:

The check is meant to make sure no raffle is still running, but it confuses two different things: the balance of the contract and the amount of fees owed. The balance can be larger than totalFees for reasons that have nothing to do with active players. The contract has no receive function, so it cannot accept a plain ether transfer, but ether can still be forced into it with selfdestruct from another contract, which bypasses any receive check. A single wei sent that way is enough. The same mismatch can also arise from rounding: the prize is 80% of the total collected and the fee is 20%, and both divisions round down, so if the total collected is not divisible by 100, up to 1 wei of dust is left behind in each round and the balance no longer matches totalFees. Once either of those things has happened, the equality can never become true again on its own, because nothing in the contract can remove the extra ether.

Risk

Likelihood: Medium - forcing ether into a contract with selfdestruct is a well-known, cheap technique that needs no permission and costs only a few wei, and the rounding dust arises from normal use whenever the entrance fee and player count do not divide neatly.
Impact: Medium - the fee address permanently loses all the fees it has earned, including fees from every past raffle round, because they can never be withdrawn. The ether is not sent to the wrong person; it is stuck in the contract with no rescue function, so it is lost to the project for good. An attacker can also use this deliberately to deny the project its income at almost no cost to themselves.

Proof of Concept

Manual trace - no tests were run; this is a step-by-step reading of the code, with a worked example.

Worked example: suppose one raffle round has finished, the prize has been paid, and the fees recorded are totalFees = 0.8 ether, with the contract balance also exactly 0.8 ether.

1. An attacker deploys a small contract that holds 1 wei and calls selfdestruct, naming the PuppyRaffle contract as the recipient. The 1 wei is forced into PuppyRaffle, even though it has no receive function.

2. The contract balance is now 0.8 ether + 1 wei, while totalFees is still 0.8 ether.

3. The owner, or the fee address, calls withdrawFees(). The first line checks address(this).balance == totalFees, which is now 0.8 ether + 1 wei == 0.8 ether, so it is false and the call reverts with "There are currently players active!", even though no players are active at all.

4. There is no function that can remove the extra wei, return it, or correct the balance. Calling withdrawFees() again changes nothing, and the 0.8 ether of fees stays locked in the contract permanently.

5. The same outcome follows from rounding dust alone: if a round collects an amount that is not divisible by 100, the prize and the fee together are 1 wei less than the total, so 1 wei of dust stays in the contract, the balance exceeds totalFees by that wei, and every later withdrawal fails in the same way.

Recommended Mitigation

Do not require the balance to equal the fees exactly. Check that no raffle is active in a way that does not depend on the balance, for example by checking that the players array is empty, and withdraw only the recorded fee amount, allowing any extra ether to stay in the contract or be handled separately.

Alternatively, keep the balance check but make it >= instead of ==, so extra ether does not block the withdrawal, and add a separate function to recover stray ether.