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

Fee Withdrawal Permanently Blocked by Strict Balance Check

firmanregar

Root + Impact

Description

The withdrawFees() function requires the contract balance to exactly equal totalFees. Any ETH sent to the contract outside of the normal flow (e.g., via selfdestruct or while players are active) permanently breaks this equality, making fees unwithdrawable forever.

The withdrawal logic uses strict equality:

require(address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!");

This assumes the contract balance only ever contains protocol fees and nothing else. In practice, this assumption is fragile:

  1. Forced ETH transfers. Anyone can force ETH into the contract via selfdestruct from another contract

  2. Active player funds. If players are active, their entrance fees inflate the balance above totalFees

  3. Rounding errors. Any accounting mismatch (like from the downcasting issue) permanently breaks the equality

Risk

Likelihood:

High probability of occurrence and results in permanent fund lock.

Impact:

Once the balance diverges from totalFees, the owner can never withdraw accumulated fees. This is particularly problematic because:

  • Forced ETH transfers are trivial and unavoidable on Ethereum

  • There's no recovery mechanism

  • All future fees become permanently locked

Proof of Concept

An attacker can lock fees with a single transaction:

contract ForceSend {
constructor(address payable target) payable {
selfdestruct(target);
}
}
​
// Deploy with:
new ForceSend{value: 1 wei}(payable(address(puppyRaffle)));

This sends 1 wei to the raffle contract without triggering any functions. Now address(this).balance != totalFees, and the equality check fails forever.

Recommended Mitigation

Track player funds separately from protocol fees, or simply allow withdrawing the known fee amount:

function withdrawFees() external {
require(totalFees > 0, "No fees to withdraw");
require(address(this).balance >= totalFees, "Insufficient balance");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

This removes the strict equality requirement while still ensuring fees are available for withdrawal.

Updates

Lead Judging Commences

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