Root + Impact

Description

• changeFeeAddress() allows the owner to set feeAddress to any address, including address(0), without validation:

• If the owner accidentally sets feeAddress to address(0), subsequent calls to withdrawFees() would send ETH to the zero address via feeAddress.call{value: feesToWithdraw}(""). While this technically succeeds (ETH is sent to address(0)), the funds are effectively burned and irrecoverable.

Risk

Likelihood:

• Low — requires owner error (accidental address(0) input)

• Only callable by the contract owner (onlyOwner modifier)

Impact:

• Low-Medium — protocol fees permanently burned if triggered

• Irrecoverable once withdrawFees() is called with feeAddress == address(0)

Proof of Concept

How the issue manifests:

1. The contract owner calls changeFeeAddress(address(0)) by mistake (e.g., passing an uninitialized variable)

2. feeAddress is now set to address(0) — the event FeeAddressChanged(address(0)) is emitted but there is no on-chain prevention

3. When withdrawFees() is called, ETH is sent to address(0) via a low-level call, which succeeds but the funds are burned

Vulnerable code (src/PuppyRaffle.sol:167-170):

Expected outcome: Protocol fees are permanently burned by being sent to address(0), with no way to recover them.

Recommended Mitigation

The root cause is that changeFeeAddress() performs no input validation on the newFeeAddress parameter. Since the owner is a human (or multisig), accidental zero-address input is a realistic operational error — especially when interacting via scripts or low-level calls where uninitialized variables default to address(0).

Primary fix — Add zero-address validation:

Why this works: The require check prevents the only dangerous input value. Since address(0) is never a valid fee recipient (ETH sent there is permanently burned), there is no legitimate use case that this check blocks.

Additional hardening — Two-step ownership transfer pattern:

For critical admin functions that change fund destinations, a two-step pattern adds an extra safety layer:

Why this helps: The two-step pattern requires the new fee address to actively confirm acceptance, which verifies: (1) the address is not zero (a zero address cannot send transactions), (2) the address is controlled by someone (they can call acceptFeeAddress), and (3) there are no typos in the address. This pattern is used by OpenZeppelin's Ownable2Step for ownership transfers.