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

Fees could be blocked for `feeAddress`

0xethanol

Vulnerability Details

By the way it is programmed the withdrawFees() function it could be possible for an bad actor to block the access to the fees for the feeAddress.

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

This is the first line of withdrawFees(), if there are no players in the raffle, this line passes, but if there are any ongoing players in the raffle, the execution of the function is blocked, a bad actor could listen to the event Transfer() which is emited after calling the _safeMint() at the end of selectWinner(), and enter the raffle just after this event with enterRaffle().

Resulting in the impossibility of withdrawing funds for the feeAddress.

Impact

High - Medium: Fund for the feeAddress could be blocked indefinitely

Recommendations

There are two things we can do to improve the code:

  1. Calculate the fees and totalAmountCollected each time a player enters or exists the raffle.

  2. Send the fees at the same time as the prizePool. (And eliminate the withdrawFees() function)

I'll explain the process of applying both solutions down below:

Solution

This solution requires to change the way some variables are computed, and it let's us at the same time to eliminate one vulnerability of the program.

We will need to change totalAmountCollected variable every time a player enters and exists the raffle in the functions enterRaflle() and refund().

We will need to eliminate this line from selectWinner():

uint256 totalAmountCollected = players.length * entranceFee;

There are two reasons we do this, the first is that is wrongly calculated, in the function refund() we are not changing the length of the players array, we are just resetting the value of where the player should be to address(0). If we leave it unchanged, if one person gets refunded the raffle will be impossible to win, as it is stated in another finding.

The following lines will need to be added to eliminate the issue:

In global scope of the contract:

uint256 totalAmountCollected;

In selectWinner():

// this line will need to be eliminated
// uint256 totalAmountCollected = players.length * entranceFee;
​

In enterRaffle():

// after checking for duplicates
totalAmountCollected += entranceFee * newPlayers.length;

In refund()

// after the require statements
totalAmountCollected -= entranceFee;

Add a line of code to select winner that would send the fees to feeAddress instead of having withdrawFees() function. It would eliminate the problem of being unable to a bad actor to purposely block the feeAddress to withdraw funds. This is the line of code in question:

(bool success,) = feeAddress.call{value: fees}("");

with this approach we can even eliminate the variable totalFees completely from the global scope and reduce the cost of deployment and execution, the final code for selectWinner() in this case would be:

​
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];
// we remove this line, as it is not necessary
// uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
// we remove this line, as it is not necessary
// 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 successWinner,) = winner.call{value: prizePool}("");
require(successWinner, "PuppyRaffle: Failed to send prize pool to winner");
​
// we add this line to send the fees, and we ensure that the transfer was succesful
// in this case we assume that the `feeAddress` is an EOA or a Smart Contract able to receive ether
(bool successFees,) = feeAddress.call{value: fees}("");
require(successFees, "PuppyRaffle: Failed to send fees to feeAddress");
​
_safeMint(winner, tokenId);
}
​
Updates

Lead Judging Commences

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