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

Strict balance equality in withdrawFees lets anyone permanently lock protocol fees via a forced ETH transfer

alphalens

Root + Impact

  • withdrawFees() requires address(this).balance to exactly equal totalFees. Anyone can force ETH into the contract via selfdestruct, breaking that equality forever, so the fee withdrawal reverts permanently and all accumulated fees are locked.

Description

  • withdrawFees() should let feeAddress withdraw the accumulated protocol fees once no prize funds remain.

  • The function gates withdrawal on address(this).balance == uint256(totalFees). A contract can be selfdestructed with the raffle as recipient, which increases the balance without going through enterRaffle, so balance permanently exceeds totalFees and the require can never pass again.

@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");

Risk

Likelihood:

  • This occurs as soon as anyone deploys a one-line contract that selfdestructs to the raffle address, which no access control can prevent.

  • The extra wei can never be removed, so once forced in, the condition is broken permanently rather than temporarily.

Impact:

  • All accumulated protocol fees become permanently unwithdrawable by feeAddress.

  • The griefing costs the attacker only 1 wei plus gas, making the denial-of-service cheap and irreversible.

Proof of Concept

  • A minimal attacker contract selfdestructs and sends 1 wei to the raffle. After that, address(this).balance is always greater than totalFees, so every withdrawFees() call reverts.

// minimal force-send contract
contract ForceSend {
constructor(address payable target) payable {
selfdestruct(target);
}
}
​
function test_ForceSendLocksFees() public {
// (assume a raffle has run so totalFees > 0 and balance == totalFees)
​
// Force 1 wei into the raffle, breaking balance == totalFees forever
new ForceSend{value: 1 wei}(payable(address(puppyRaffle)));
​
vm.prank(feeAddress);
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees(); // permanently reverts
}

Recommended Mitigation

  • Do not tie withdrawal to an exact balance. Withdraw the recorded totalFees directly and reset it, so an inflated balance cannot block withdrawal.

- 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 about 5 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!