First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

`PuppyRaffle::withdrawFees` balance check can cause fees to be stuck

maroutis

Summary

Inside the PuppyRaffle::withdrawFees function, the require checks if balance is equal to totalFees. However, various scenarios, which might not necessarily arise from malicious intent, can cause this check to consistently fail. Also, the fact that players can enter the raffle right after a winner selection makes it very difficult to time the right moment for the fees withdrawal and/or any amounts/dust amounts remaining.

Vulnerability Details

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

Potential issues could arise from:

  • Any remaining dust amounts due to the rouding of the division :

uint256 fee = (totalAmountCollected * 20) / 100;

  • Accidental or intentional transfers of funds directly to the contract.

  • Players entering the raffle all the time as there is no pause period after winner selection that prevent entering the raffle.

These scenarios can cause a DOS where the function PuppyRaffle::withdrawFees is completely inoperable.

Impact

Fees are completely stuck with no way to recover. High impact for the protocol, as there will be no revenue.

Proof of concept

To execute this test : forge test --mt testWithrawFeesRevertsAfterAdding1Wei -vvvv

function testWithrawFeesRevertsAfterAdding1Wei() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
// 1 wei remains in the contract
// Either someone deliberately send it
// Or someone mistakenly sends it
// Rounding errors from the fee calculation
vm.deal(address(puppyRaffle), 1 wei);
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Tools Used

Manual review

Recommendations

  • The require is not needed, as only the fees will be sent.

- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

  • Add an onlyOwner modifier to the function PuppyRaffle::withdrawFees

  • Add a an onlyOwner function to retrieve any leftover amounts. But in order to not withdraw the player's amount, there needs to be a brief pause after a winner is selected. Consider adding this requirement in the function PuppyRaffle::enterRaffle

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

greifers-send-money-to-contract-to-block-withdrawfees

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!