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

Incorrect Balance Check in `withdrawFees()`

muzammilmohammed678

Root + Impact

Description

  • * The `withdrawFees()` function is intended to allow the owner to withdraw accumulated fees when no players are active. It checks that the contract balance exactly equals the tracked `totalFees`.

    * The function uses an exact equality check `address(this).balance == uint256(totalFees)`, which fails to account for several scenarios: direct ETH transfers to the contract (via `selfdestruct`), rounding errors from prize/fee calculations, or any other source of ETH that doesn't update `totalFees`.

    ```solidity:157:163:src/PuppyRaffle.sol

    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

Likelihood:

  • * Rounding errors in `selectWinner()` (see H-6) will cause leftover wei to accumulate, making the balance higher than `totalFees`

    * Anyone can send ETH directly to the contract address (via `selfdestruct` or as recipient), which increases balance without updating `totalFees`

    * The exact equality check will fail whenever any of these scenarios occur

Impact:

  • * Legitimate fees become permanently locked and unwithdrawable

    * Owner cannot access funds that should be withdrawable

    * Contract balance grows with unwithdrawable funds over time

    * Accounting becomes permanently broken if direct ETH transfers occur

Proof of Concept

```solidity
// Scenario 1: Rounding error
// After selectWinner() with rounding, balance = totalFees + 1 wei
// withdrawFees() will revert due to exact equality check
​
// Scenario 2: Direct ETH transfer
// Attacker sends 1 wei to contract via selfdestruct
// balance = totalFees + 1 wei
// withdrawFees() permanently blocked
```

Recommended Mitigation

```diff
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(players.length == 0, "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");
}
```
​
Better approach: Track active players separately:
​
```diff
+ uint256 public activePlayersCount;
+
function enterRaffle(address[] memory newPlayers) public payable {
// ... existing code ...
+ activePlayersCount += newPlayers.length;
}
​
function refund(uint256 playerIndex) public {
// ... existing code ...
+ activePlayersCount--;
}
​
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(activePlayersCount == 0, "PuppyRaffle: There are currently players active!");
// ... rest of function
}
```
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 16 days 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!