Summary
The potential function is subjected to reentrancy and can easily be compromised through external attacks.
Vulnerability Details
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
uint256 tokenId = totalSupply();
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
if (rarity <= COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
} else if (rarity <= COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
@> (bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
}
In the selectWinner function, the external call is made with (bool success,) = winner.call{value: prizePool}(""); before the state of the contract is fully updated with _safeMint(winner, tokenId).
Impact
In the selectWinner function, the external call is made before the state of the contract is fully updated. If winner is a contract, it could potentially call selectWinner again before the first call has finished.
Here is my proof of code of test:
Copy paste the test in your HardHat setup
const { expect } = require("chai");
describe("PuppyRaffle Reentrancy Attack Test", function () {
it("should demonstrate the reentrancy attack", async function () {
const PuppyRaffle = await ethers.getContractFactory("PuppyRaffle");
const puppyRaffle = await PuppyRaffle.deploy();
await puppyRaffle.deployed();
const MaliciousContract = await ethers.getContractFactory("MaliciousContract");
const maliciousContract = await MaliciousContract.deploy(puppyRaffle.address);
await maliciousContract.deployed();
await maliciousContract.triggerReentrancyAttack();
const balance = await ethers.provider.getBalance(puppyRaffle.address);
expect(balance).to.equal(expectedBalance);
});
});
In the deployment script, deploy both the PuppyRaffle and MaliciousContract contracts:
async function main() {
const PuppyRaffle = await ethers.getContractFactory("PuppyRaffle");
const puppyRaffle = await PuppyRaffle.deploy();
await puppyRaffle.deployed();
const MaliciousContract = await ethers.getContractFactory("MaliciousContract");
const maliciousContract = await MaliciousContract.deploy(puppyRaffle.address);
await maliciousContract.deployed();
console.log("PuppyRaffle deployed to:", puppyRaffle.address);
console.log("MaliciousContract deployed to:", maliciousContract.address);
}
main()
.then(() => process.exit(0))
.catch((error) => {
console.error(error);
process.exit(1);
});
In the MaliciousContract.sol file, implement the malicious contract with a function that exploits the reentrancy vulnerability:
pragma solidity ^0.7.6;
contract MaliciousContract {
address public puppyRaffleAddress;
constructor(address _puppyRaffleAddress) {
puppyRaffleAddress = _puppyRaffleAddress;
}
function triggerReentrancyAttack() public {
}
}
Finally run the HardHat test which will show that this function is vulnerable to Reentrancy Attack.
Tools Used
Remix IDE, Hardhat
Recommendations
Follow the “Checks-Effects-Interactions” pattern, which suggests you perform any external calls or interactions last, after all internal work is done:
You can replace this function code with your function code of the function "selectWinner" for preventing the Reentrancy
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
uint256 tokenId = totalSupply();
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
if (rarity <= COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
} else if (rarity <= COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
_safeMint(winner, tokenId);
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
}
