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

`Reentrancy` on `refund()` function because of state changes after `sendValue()` function

naman1729

Summary

  • refund() function can be called multiple times in a single transaction before the state changes which can drain the contract balance. This attack is a classic reentrancy attack and can be prevented by changing state before the external call ("sendValue()") to prevent from reentrancy attack.

Vulnerability Details

Click this to see Code
​
/// @param playerIndex the index of the player to refund. You can find it externally by calling `getActivePlayerIndex`
/// @dev This function will allow there to be blank spots in the array
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);
}
​

  • Reentrancy is possible because of state changes after sendValue() function which can drain the contract balance by calling refund() function multiple times in a single transaction before the state changes.

Impact

  • Drain contract balance by calling refund() function multiple times in a single transaction before the state changes.

Tools Used

  • Manual Review

Recommendations

/// @param playerIndex the index of the player to refund. You can find it externally by calling `getActivePlayerIndex`
/// @dev This function will allow there to be blank spots in the array
function refund(uint256 playerIndex) public {
+ require(playerIndex < players.length, "PuppyRaffle: Player index out of bounds");
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");
​
+ players[playerIndex] = players[players.length-1];
+ players.pop();
​
payable(msg.sender).sendValue(entranceFee);
​
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
​

  • This is a common reentrancy pattern to prevent reentrancy attack. It is recommended to use this pattern.

  • It saves from reentrancy attack and maintains the array length.

Updates

Lead Judging Commences

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

reentrancy-in-refund

reentrancy in refund() function

Support

FAQs

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