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

[M-2] Flawed Guard Condition in `withdrawFees` Prevents Fee Withdrawal

0xaljzoli

[M-2] Flawed Guard Condition in withdrawFees Prevents Fee Withdrawal

Title: Flawed Guard Condition in withdrawFees Prevents Fee Withdrawal

Impact: Medium

Likelihood: High

Description

  • The withdrawFees function allows the owner to withdraw accumulated protocol fees to the feeAddress. The intent is that fees can only be withdrawn when no active players are in the raffle, ensuring that player funds are never mixed with or accidentally withdrawn as fees.

  • The guard condition require(address(this).balance == uint256(totalFees)) is designed to verify that the contract holds exactly the amount of fees and no player funds. However, this condition is extremely brittle. If anyone sends even a single wei of ETH to the contract via a plain transfer (self-destruct, mining reward, or accidental send), the balance no longer matches totalFees and fee withdrawal is permanently blocked. Additionally, totalFees is stored as a uint64, which can overflow if the protocol accumulates more than ~18.4 ETH in fees, causing the guard to fail due to truncation.

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ // @> Requires EXACT match — any stray wei blocks withdrawal
+ // @> uint64 totalFees truncates at ~18.4 ETH, causing mismatch
​
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Risk

Likelihood:

  • Anyone can send ETH directly to the contract address at any time. Ethereum allows forced ETH sends via selfdestruct — an attacker can destroy a contract holding even 1 wei and send it to the PuppyRaffle address, immediately locking fee withdrawals forever.

  • The uint64 overflow occurs naturally as the protocol scales. uint64 has a max value of ~18.44 ETH. After approximately 92 raffle rounds with 4 players at 1 ETH each (accumulating 0.8 ETH in fees per round), totalFees wraps around, making the guard condition impossible to satisfy.

Impact:

  • When the guard condition fails, the owner cannot withdraw any accumulated fees. Protocol revenue is permanently locked in the contract. If the condition fails due to a selfdestruct dust attack, the fix requires a contract upgrade or a complex recovery mechanism.

  • If the condition fails due to uint64 overflow, totalFees silently wraps to a small number while the contract holds the actual fees. The withdrawFees function would then only attempt to withdraw the truncated amount, sending a fraction of the real fees to the owner, with the remainder permanently locked.

Proof of Concept

function testWithdrawFeesBlockedByDust() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// Select winner — fees accumulate
puppyRaffle.selectWinner();
​
// Attacker sends 1 wei to the contract via selfdestruct
AttackerForceSend attacker = new AttackerForceSend{value: 1}(address(puppyRaffle));
​
// Contract balance is now: fees + 1 wei
// totalFees = (4 * 1e18 * 20 / 100) = 0.8 ETH
// address(this).balance = 0.8 ETH + 1 wei
// Guard fails: 0.8 ETH != 0.8 ETH + 1 wei
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
contract AttackerForceSend {
constructor(address target) payable {
selfdestruct(payable(target));
}
}

This PoC demonstrates two attack vectors. First, an attacker uses selfdestruct to force-send 1 wei to the contract. Since selfdestruct cannot be prevented by any contract, even contracts without a receive() function can be force-funded. After this single wei arrives, the exact equality check in withdrawFees permanently fails because address(this).balance (which includes the dust wei) no longer equals totalFees. Second, the uint64 truncation means that after accumulating more than 18.44 ETH in total fees across all rounds, the stored totalFees value silently overflows and wraps around, making the guard condition unsatisfiable even without any external attack.

Recommended Mitigation

+ uint256 public totalFees = 0;
​
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(totalFees > 0, "PuppyRaffle: No fees to withdraw");
​
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Two changes are recommended. First, change totalFees from uint64 to uint256 to eliminate the overflow risk at ~18.44 ETH. Second, replace the fragile exact equality check with a simple require(totalFees > 0). The original intent of the guard was to prevent fee withdrawal while players are active, but this is already enforced by the fact that selectWinner resets the round and totalFees only accumulates after a successful selectWinner call. If needed, a separate boolean flag bool public raffleActive can be added and set to false in selectWinner and true in enterRaffle, providing a clean and unbreakable guard condition.

Updates

Lead Judging Commences

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