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

Strict balance equality check in `PuppyRaffle::withdrawFees` can permanently lock protocol fees

frozen

Strict balance equality check in PuppyRaffle::withdrawFees function allows permanent locking of fees via forced ETHER

Description

Normal behavior:
The PuppyRaffle::withdrawFees function is intended to allow the protocol owner to withdraw accumulated raffle fees totalFees after a raffle has completed, prizePool has been transferred to the winner and only protocol fees remain in the contract balance.

Issue:
The function relies on a strict equality check between address(this).balance and totalFees to determine whether fees can be withdrawn. However, a contract’s ETH balance can be increased by any external party without executing contract logic, for example via selfdestruct.

If an attacker forcibly sends ETH to the contract, address(this).balance will no longer equal totalFees, causing the strict equality check to permanently fail. As a result, withdrawFees becomes unusable and protocol fees are permanently locked.

​
function withdrawFees() external {
@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
...
}

Risk

Likelihood:

  • Any external account can forcibly send ETH to the contract (e.g., via selfdestruct) without executing contract logic, making the strict balance equality check fail reliably.

  • The attack does not require special permissions, timing assumptions, or interaction with protocol functions.

Impact:

  • Protocol fees can become permanently locked and unrecoverable.

  • Results in a denial of service on protocol revenue, preventing the contract owner from withdrawing earned fees.

Proof of Concept

  • Assume PuppyRaffle has completed a raffle and totalFees > 0.

contract ForceEther {
constructor() payable {}
​
function attack(address target) external {
selfdestruct(payable(target));
}
}

  • An attacker deploys ForceEther with a small amount of ETH.

  • The attacker calls ForceEther::attack(PuppyRaffleAddress), forcibly sending ETH to the PuppyRaffle contract.

  • The contract balance now exceeds totalFees.

  • Any subsequent call to PuppyRaffle::withdrawFees reverts because address(this).balance != totalFees.

As a result, protocol fees become permanently locked.

Recommended Mitigation

A. Avoid using strict equality checks on address(this).balance, as contract balances can be increased via forced ETH transfers.
Replace the strict equality check with a >= comparison to ensure protocol fees remain withdrawable even if extra ETH is sent to the contract.

- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(address(this).balance >= uint256(totalFees), "PuppyRaffle: Insufficient balance for fees");

B. Track state of protocol explicitly instead of inferring it from balance.
Introduce an explicit state variable to track raffle finalization i.e. PuppyRaffle::raffleFinalized, and update it once the raffle has completed and the prize has been distributed.
We can now use this state as a check if raffle is finished or not .

bool public raffleFinalized = false;

Set raffleFinalized = true at the end of PuppyRaffle::selectWinner after the prize payout.

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(raffleFinalized, "Raffle not finalized");
...
}

This approach avoids relying on externally mutable balance values.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 2 days 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!