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

Invalid players handling in refund() leads to deadlock in the protocol

0xrochimaru

Summary

If two or more players call refund() then it'll introduce a duplicate in the players array resulting in revert of enterRaffle() method and if the number of players in less than 4 then selectWinner() cannot be called either. Resulting in the deadlock in the protocol.

Vulnerability Details

The refund() method replaces the address of the player with address(0). If two or more players refunds then it introduces a duplicate in the players array in the form of address(0).

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

Now, when the enterRaffle() is called, it checks the duplicates and if duplicate is found, it reverts the transaction. Because the refund() intoduced duplicates in the players array. Which causes enterRaffle() to revert no matter what input param is sent.

function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
}
​
// Check for duplicates
for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
require(players[i] != players[j], "PuppyRaffle: Duplicate player");
}
}
emit RaffleEnter(newPlayers);
}
​

Now, If the size of the players array is <=4. It causes selectWinner() to revert because of the below check. Resulting in a deadlock in the protocol.

require(players.length >= 4, "PuppyRaffle: Need at least 4 players");

POC Test:

function testCanEnterRaffleFailure() public {
address[] memory players = new address[](3);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
puppyRaffle.enterRaffle{value: entranceFee * 3}(players);
console.log("3 players enter the raffle");
vm.prank(playerOne);
puppyRaffle.refund(0);
console.log("playerOne left");
vm.prank(playerTwo);
puppyRaffle.refund(1);
console.log("playerTwo left");
delete players;
players = new address[](1);
players[0] = playerFour;
​
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(players);
console.log("Txn reverts as playerFour tries to enterRaffle");
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
vm.expectRevert("PuppyRaffle: Need at least 4 players");
puppyRaffle.selectWinner();
console.log("Txn reverts if selectWinner is called");
}
​

Logs

3 players enter the raffle
playerOne left
playerTwo left
Txn reverts as playerFour tries to enterRaffle
Txn reverts if selectWinner is called

Impact

Whole protocol will hault and user's will not be able to enter raffle and select winners.

Recommendations

Decrease the size of players array as user calls refund().

if(players.length > 1) {
players[playerIndex] = players[players.length - 1];
players.pop();
} else {
delete players;
}
Updates

Lead Judging Commences

Hamiltonite Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

refund-doesnt-reduce-players-array-size-causing-protocol-to-freeze

zero address can win the raffle

Funds are locked to no one. If someone gets the refund issue, they also got this issue. IMPACT: High Likelihood: High

Support

FAQs

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