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

Reentrancy in PuppyRaffle.refund() Allows Duplicate Refunds

zanycat871

Description

The PuppyRaffle.refund() function is vulnerable to reentrancy because it transfers ETH to the caller before updating the corresponding entry in the players array.

When sendValue() transfers ETH to a malicious contract, the recipient can execute its receive() callback and re-enter refund() before the original call sets players[playerIndex] to address(0).

The root cause is the incorrect ordering of the external interaction and state update in refund().

File: `src/PuppyRaffle.sol`
​
```solidity
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"
);
​
// @> Vulnerability: external ETH transfer occurs before state update
payable(msg.sender).sendValue(entranceFee);
​
// @> The player's entry is invalidated too late, enabling reentrancy
players[playerIndex] = address(0);
​
emit RaffleRefunded(playerAddress);
}
```
​
**Explanation:** The contract transfers ETH to `msg.sender` before setting `players[playerIndex]` to `address(0)`. A malicious recipient can re-enter `refund()` from its `receive()` callback while the original entry is still active, potentially receiving multiple refunds for the same entry.
​

Risk

Likelihood: High

  • Reason 1: The vulnerability is triggered when a participant calls refund() and the ETH transfer invokes a malicious contract's receive() callback before the player's entry is marked as refunded.

  • Reason 2: An attacker can use a contract that re-enters refund() during the callback. The proof-of-concept test demonstrates that the reentrant call succeeds when the raffle contract holds sufficient ETH.

Impact: High

  • Impact 1: An attacker can receive multiple refunds for a single raffle entry, resulting in unauthorized ETH transfers from the raffle contract.

  • Impact 2: The exploit can cause financial losses for other participants by reducing the ETH available to the raffle and potentially preventing legitimate refunds or other payouts.

Proof of Concept

The vulnerability was reproduced using a Foundry test named testReentrancyRefund.

Setup

  • Deploy PuppyRaffle with an entrance fee of 1 ETH.

  • Deploy a malicious contract that calls refund(0) and attempts to re-enter the same function from its receive() callback.

  • Register the malicious contract as the first participant.

  • Register two additional participants, each paying 1 ETH.

  • Confirm that the raffle contract holds 3 ETH before the attack.

Execution

The malicious contract calls attack(0), which invokes refund(0).

The raffle transfers 1 ETH to the malicious contract before invalidating the player's entry. During the ETH transfer, the malicious contract's receive() callback calls refund(0) again. The nested call succeeds because the original entry has not yet been set to address(0).

Observed Result

The Foundry test passed against the vulnerable implementation and confirmed that:

  • The attacker received 2 ETH from a single entry.

  • The reentrant call succeeded.

  • The raffle initially held 3 ETH, so sufficient funds were available for both refunds.

Expected Result

Each valid entry should be refunded at most once. The second call to refund(0) should fail after the entry has been invalidated.

## Proof of Concept (PoC)
​
### Test Scenario
​
1. Deploy `PuppyRaffle` with an entrance fee of 1 ETH.
2. Deploy a malicious contract that calls `refund(0)` and attempts to re-enter `refund(0)` from its `receive()` callback.
3. Register the malicious contract as the first participant, followed by two additional participants. Each participant pays 1 ETH, leaving the raffle contract with 3 ETH.
4. Call `attack(0)` on the malicious contract.
5. Observe that the attacker receives 2 ETH for a single raffle entry.
​
### Expected Result
​
The participant should receive only one refund of 1 ETH for their entry.
​
### Actual Result
​
The malicious contract successfully re-enters `refund(0)` before the player's entry is marked as refunded. The Foundry test confirms that the attacker receives 2 ETH from a single entry.
​
### Verification
​
The test `testReentrancyRefund` passed against the vulnerable implementation.
​
The issue is caused by transferring ETH before updating `players[playerIndex]` to `address(0)`.
​
​

Recommended Mitigation

Apply the Checks-Effects-Interactions pattern in the refund() function by updating the player's state before transferring ETH.

Suggested Fix

Replace the vulnerable code:

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

With the following:

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

Explanation

By setting players[playerIndex] to address(0) before the external ETH transfer, the player's entry is invalidated before the recipient's callback can execute. Any reentrant call attempting to refund the same entry will fail the existing validation checks.

As an additional defense-in-depth measure, consider using a reentrancy guard such as OpenZeppelin's ReentrancyGuard and applying nonReentrant to refund().

```diff
- 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 2 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!