Description
The PuppyRaffle::withdrawFees function lacks access control, allowing any address to trigger fee withdrawal to the feeAddress. According to the protocol specification, the Owner role should control fee management (through changeFeeAddress), but any Player can force the protocol to withdraw fees at any time.
Expected behavior: Based on the role model, only the Owner should control when fees are withdrawn, similar to how changeFeeAddress has onlyOwner protection.
Actual behavior: Any address can call withdrawFees() and force the protocol to send accumulated fees to feeAddress at any time.
function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Risk
Likelihood:
Impact:
-
Owner loses control over fee withdrawal timing
-
Violates principle of least privilege
-
Potential tax/accounting issues if owner wants specific withdrawal timing
-
If feeAddress is a contract, unexpected ETH transfers could disrupt its logic
-
Griefing: Malicious actors can force withdrawals at inconvenient times
Note: While funds are not stolen (they go to the correct feeAddress), the loss of control over timing violates the specified role model.
Proof of Concept
Add to test/PuppyRaffleTest.t.sol:
* @notice Demonstrates that anyone can call withdrawFees, violating access control
* @dev The spec implies only Owner controls fees, but any player can force withdrawal
*/
function test_anyoneCanWithdrawFees() public {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
vm.deal(playerOne, entranceFee * 4);
vm.prank(playerOne);
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
uint256 feesAccumulated = uint256(puppyRaffle.totalFees());
uint256 feeAddressBalanceBefore = address(feeAddress).balance;
emit log("=== Access Control Test ===");
emit log_named_address("Owner (deployer)", address(this));
emit log_named_address("Fee address", feeAddress);
emit log_named_uint("Fees accumulated", feesAccumulated);
emit log("");
address randomPlayer = address(999);
emit log_named_address("Random player calling withdrawFees", randomPlayer);
vm.prank(randomPlayer);
puppyRaffle.withdrawFees();
emit log("");
emit log("=== Result ===");
emit log("SUCCESS: Random player withdrew fees!");
emit log_named_uint("Fee address balance after", address(feeAddress).balance);
emit log("");
emit log("=== Problem ===");
emit log("withdrawFees has no access control");
emit log("Any player can force fee withdrawal at any time");
emit log("Owner loses control over WHEN fees are withdrawn");
assertEq(address(feeAddress).balance, feeAddressBalanceBefore + feesAccumulated);
assertEq(uint256(puppyRaffle.totalFees()), 0);
}
Run: forge test --match-test test_anyoneCanWithdrawFees -vv --offline
Output:
=== Access Control Test ===
Owner (deployer): 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
Fee address: 0x0000000000000000000000000000000000000063
Fees accumulated: 800000000000000000
Random player calling withdrawFees: 0x00000000000000000000000000000003E7
=== Result ===
SUCCESS: Random player withdrew fees!
Fee address balance after: 800000000000000000
=== Problem ===
withdrawFees has no access control
Any player can force fee withdrawal at any time
Owner loses control over WHEN fees are withdrawn
A completely random player (address 0x3E7) successfully triggered fee withdrawal despite not being the owner.
Recommended Mitigation
Add onlyOwner modifier to restrict fee withdrawal to the owner:
- function withdrawFees() external {
+ function withdrawFees() external onlyOwner {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
This aligns with the role model where the Owner controls fee management.
