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

Stuck ETH due to strict equality check in withdrawFees

bosz

Root + Impact

Description

  • The withdrawFees function enforces a strict equality check: require(address(this).balance == uint256(totalFees)). This logic is flawed because address(this).balance includes all ETH in the contract, including players' stakes. The intention seems to be to ensure no players are active, but even if the players array is reset, any dust or forced ETH sent to the contract (via selfdestruct) will make balance > totalFees. This permanently causes the check to fail, locking the fees in the contract forever.

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • Reason 1: Any user can send 1 wei to the contract via selfdestruct (or if it had a fallback) to grief the admin.

  • Reason 2: The check also prevents withdrawal while a raffle is active, which might be intended, but the implementation is brittle.

Impact:

  • mpact 1: The protocol owner can NEVER withdraw fees if the balance is mismatched.

  • Impact 2: Funds are permanently frozen.

Proof of Concept

function testWithdrawFeesDoS() public {
// ... Setup raffle and finish it ...
puppyRaffle.selectWinner();
// Attack: Send 1 wei to contract
SelfDestructor attacker = new SelfDestructor(payable(address(puppyRaffle)));
attacker.attack{value: 1}();
// Now balance > totalFees
vm.expectRevert(bytes("PuppyRaffle: There are currently players active!"));
puppyRaffle.withdrawFees();
}

Recommended Mitigation

//Do not rely on address(this).balance equality. Allow withdrawing up to the totalFees amount, or simply withdraw totalFees.
​
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");
}
Updates

Lead Judging Commences

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