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

refund does not follow the CEI design pattern - resulting in a potential loss of funds

coffee

Summary

The 'PuppyRaffle::refund' function does not follow the CEI 'Checks-Effects-Interactions' design pattern. This can lead to risk of funds or reentrancy attacks.

Vulnerability Details

Because refund sends the player their entrance fee before removing them from the players array, this is susceptible to a reentrancy attack. However, send does have a gas limit of 2300 units, this should prevent a reentrancy attack from happening because there would not be enough gas to recursively call back into the function to exploit the funds.

function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
​
@> payable(msg.sender).sendValue(entranceFee);
​
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Impact

Unlikely, but potential loss of funds due to reentrancy attack. Regardless of the gas limit of send, this function should follow the CEI design pattern.

Tools Used

-Foundry

Recommendations

To follow the CEI design pattern, these two lines should be swapped. The players array should be updated before sending any value back to the player.

function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
- payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
​
+ players[playerIndex] = address(0);
+ payable(msg.sender).sendValue(entranceFee);
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Invalidated
Reason: Vague generalities

Support

FAQs

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

Give us feedback!