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

Inadequate Defense Mechanisms in selectWinner Resulting in Potential Reentrancy Vulnerabilities

Osora9

Summary

The selectWinner function in the PuppyRaffle contract has been identified with a potential reentrancy vulnerability. This attack happens when a function is invoked again before it completes its execution.

Vulnerability Details

The function is susceptible to reentrancy attacks under specific conditions. However, several operations within the selectWinner function can mitigate or nullify the effects of such an attack:

  • delete players;: Clears the list of players before the function completes, limiting the impact during an attack.

  • Reset of block.timestamp: The reset of lastMintedBlockNumber and block.timestamp makes it harder to predict the random number generation for the next round.

  • _safeMint: This function is designed to deny minting if the specified token ID already exists. This ensures that an attacker cannot mint the same token multiple times using a reentrancy attack.

Impact

Given the current implementation, it's believed that no severe damages would result from a reentrancy attack. However, reentrancy attacks typically have multiple unforeseen side effects, and verifying that the code has adequate protection against these side effects is essential.

POC

Attack

contract Attack is Test {
PuppyRaffle public puppyRaffle;
address[] public players = new address[](5);
​
constructor(address _puppyRaffle) {
puppyRaffle = PuppyRaffle(_puppyRaffle);
}
​
function winnerIndexAttack() public {
setUpToEnterRaffle();
uint256 winnerIndex = uint256(keccak256(abi.encodePacked(address(this), block.timestamp, block.difficulty))) % 5; //players.length=5
console.log("winnerIndex: %s", winnerIndex);
if (winnerIndex == puppyRaffle.getActivePlayerIndex(address(this))) {
puppyRaffle.selectWinner();
}
}
​
function onERC721Received(address, address, uint256, bytes calldata) external pure returns (bytes4) {
return bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"));
}
​
function setUpToEnterRaffle() public {
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
players[4] = address(this);
puppyRaffle.enterRaffle{value: puppyRaffle.entranceFee() * 5}(players);
}
​
fallback() external payable {}
​
receive() external payable {
if (address(puppyRaffle).balance >= 1 ether && address(this).balance >= 1 ether) {
//Somehow manipulate the timestamp
vm.warp(block.timestamp + puppyRaffle.raffleDuration() + 1230);
setUpToEnterRaffle();
puppyRaffle.selectWinner();
}
}
}

Test

​
function testReentrancyOnSelectWinner() public {
vm.warp(block.timestamp + duration + 1230); //manipulate the block.timestamp
vm.roll(block.number + 1);
deal(address(attack), 15 ether);
vm.expectRevert("ERC721: token already minted");
attack.winnerIndexAttack();
}

Tools Used

Foundry

Recommendations

Use the reentrancyGuard modifier from OpenZeppelin to protect against reentrancy attacks. Implement it as follows:

- contract PuppyRaffle is ERC721, Ownable{
- function selectWinner() external {
​
+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+ contract PuppyRaffle is ERC721, Ownable, ReentrancyGuard {
+ // ... (other parts of the contract)
+
+ function selectWinner() external nonReentrant {
+ // ... (rest of the function)
+ }
​

By using nonReentrant, the function will revert if there's a recursive call, preventing the reentrancy attack.

Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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