Puppy Raffle

Description

The PuppyRaffle::withdrawFees function is intended to allow the owner to withdraw accumulated fees to the feeAddress. However, the function lacks any access control modifier (such as onlyOwner). This means any external account can call withdrawFees() at any time, provided the condition address(this).balance == uint256(totalFees) is met (i.e., when there are no active players and the contract holds exactly the fee amount). This violates the role specification documented in the project, where only the Owner should have the power to manage fee withdrawals.

Risk

Likelihood: Medium – The function can only be called when the players array is empty (after a raffle has finished and selectWinner has reset it). However, once that condition is met, any attacker can front‑run or simply invoke the function before the owner does.

Impact: High – The owner loses control over the timing of fee withdrawals. An attacker can withdraw fees prematurely, potentially disrupting operational plans (e.g., if the feeAddress is a multisig that requires preparation). In a worst‑case scenario where the feeAddress is compromised or maliciously changed, an attacker could instantly drain the fee balance. Moreover, this contradicts the documented role of the Owner as the sole entity responsible for fee management.

Proof of Concept

The following test demonstrates that an account other than the owner can successfully call withdrawFees() after the raffle ends:

Run the test:

Output (relevant section):

The test passes, confirming that an arbitrary address (attacker) can withdraw the fees without being the owner.

Recommended Mitigation

Add the onlyOwner modifier to the withdrawFees() function, matching the access control already used in changeFeeAddress(). This ensures that only the contract owner can trigger fee withdrawals, aligning with the documented roles.