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

`address(this).balance` in `withdrawFees()` causes stuck fees in the contract

Nocturnus

Summary

An equality check between address(this).balance and totalFees is performed before transferring the totalFee amount to the caller.

Vulnerability Details

A fixed fee is charged to each player for entering the raffle game, and the fees are accumulated in the contract. 80% of the total fee is stored as the prize pool to be awarded to the winner of each game, while the remaining 20% is stored as a protocol fee within the contract. The protocol fee is accumulated until the withdraw() function is invoked. Upon calling the selectWinner() function, the prize pool is transferred to the raffle winner, and the remaining 20% is added to totalFees.

The logic within the withdraw() function assumes that totalFees will always match the contract's balance. This logic is flawed if the contract's balance changes, as illustrated in the following scenarios:

  • Assuming there were initially four players in the raffle, and one of them is refunded, the actual number of players would be three, yet the length of the players' array would remain at four. Refunded player's index will be set to the zero address. However, the player will not be removed from the list of players, resulting in the length of the players' array remaining one more than the actual number of players.

uint256 totalAmountCollected = players.length * entranceFee; // 4 * 1 = 4
uint256 prizePool = (totalAmountCollected * 80) / 100; // 4 * 80 / 100 = 3.2
uint256 fee = (totalAmountCollected * 20) / 100; // 4 * 20 / 100 = 0.8
totalFees = totalFees + uint64(fee); // 0 + 0.8 = 0.8

According to above calculations, totalFees gets effected due to miscalculation of the players array.

address(this).balance == uint256(totalFees)

The calculations above demonstrate that totalFees is affected due to miscalculations stemming from the players' array discrepancy.

Additionally, there are two other methods to transfer balance to a contract, like in our case, if the contract lacks a receive() or fallback() function: selfdestruct() and sending balance before the contract's creation.

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

Impact

Utilizing either of the aforementioned options will result in the fees being trapped within the contract indefinitely. Proof of concept for the exploit:

function testTest2() public playersEntered {
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(indexOfPlayer);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
console.log("Contract Balance Before selectWinner():", address(puppyRaffle).balance);
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Tools Used

  • Manual code review

  • Foundry

Recommendations

The primary purpose of the equality check is to verify the presence of active players. Consider implementing an additional mechanism to determine whether the raffle is OPEN/ONGOING/CLOSED, possibly using an enum.

Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 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.