Strict balance equality in withdrawFees can be broken by forced ETH, locking fees forever
Root + Impact
Description
The owner should be able to withdraw collected fees once a round is over.
withdrawFeesrequiresaddress(this).balance == uint256(totalFees)exactly. ETH can be forced into the contract without calling it (e.g.selfdestruct, which works on 0.7.6 and still sends ETH), so a 1 wei donation makes the balance permanently different fromtotalFees.
Attack path:
A round finishes,
selectWinnersetstotalFeesand clearsplayers; balance == totalFees.Attacker deploys
Forcer{value: 1}whichselfdestructs into PuppyRaffle (noreceiveneeded).Now
address(this).balance == totalFees + 1and therequireinwithdrawFeesfails.Every later round adds the same amount to both the balance and
totalFees, so the difference of 1 wei stays forever and fees can never be withdrawn.
Risk
Likelihood:
Costs the attacker 1 wei and one transaction; the contract has no way to absorb unexpected ETH.
Impact:
Fees are locked forever (the check can never match again; new rounds only add matching amounts on both sides). This check also blocks withdrawal during active rounds, so it is a fragile design in general.
Proof of Concept
All tests run in a test contract that deploys PuppyRaffle(1e18, address(99), 1 days) and uses the helpers _players(start,n) (distinct addresses) and _enter(p) (pays 1e18 * p.length). Full file: test/AuditPoC.t.sol, forge test --mc AuditPoC passes.
Recommended Mitigation
Do not compare against the raw balance. Withdraw totalFees directly (and track active-player funds separately if you need a guard):
Lead Judging Commences
[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"); } ```
Rewards breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.