Summary
In refund() method as user exits their address is set to address(0) but the size of array in not decreased. Which results in the selectWinner() and withdrawFees() to revert forever.
Vulnerability Details
When a user calls refund() method, the entranceFee is sent to them and their adderess is set to address(0). But the size of array is not decreased.
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 selectWinner() will be called it'll calculate totalAmountCollected = players.length * entranceFee;. As the players.length is unchanged. It'll expect more totalAmountCollected than the balance of the contract. Which will result in the revert() when (bool success,) = winner.call{value: prizePool}(""); is called. Resulting in user's funds getting stuck in the contract.
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);
}
If there are a large number of player in the raffle then it is possible that the selectWinner() will execute as the prizePool is 80% of the expected contract balance. But the left contract balance will be < calculated totalFees. The fees will also be stuck in the raffle contract.
function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Example scenario:
Scenario 1:
There are 4 players in the raffle and Alice decides to exit the raffle and calls refund() method. The entranceFee is refund to the Alice but the size of array remains 4.
Mallory calls selectWinner(). Here totalAmountCollected is expected to be 4 * entranceFee but the total amount in the contract is 3 * entranceFee. Let's say the entrance fee is 1 Ether. The prizePool will be (4 * 80)/100 = 3.2 Ether. The total amount present in the contract is 3 Ether. The method (bool success,) = winner.call{value: prizePool}(""); is return false and the transaction will revert.
Scenario 2:
If we take a scenario where there were 6 players and Alice leave the pool, The prizePool will be (6 * 80)/100 = 4.8 and the contract will have 5 Ether. So the selectWinner() will execute. But the totalFees will be set to 1.2 Ether.
Now, The withdrawFees() will revert as the contract has 0.2 Ether but the total Fees is 1.2 Ether.
POC Test:
Scenario 1:
function testSelectWinnerFails() public playersEntered {
console.log("Executing Select Winner with 4 players");
console.log("Length of the players List: ", puppyRaffle.getPlayersLength());
console.log("Contract Eth Balance: ", address(puppyRaffle).balance);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
console.log("Alice decides to exit");
vm.prank(playerOne);
puppyRaffle.refund(0);
console.log("Length of the players List: ", puppyRaffle.getPlayersLength());
console.log("Contract Eth Balance: ", address(puppyRaffle).balance);
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}
Logs
Executing Select Winner with 4 players
Length of the players List: 4
Contract Eth Balance: 4000000000000000000
Alice decides to exit
Length of the players List: 4
Contract Eth Balance: 3000000000000000000
Scenario 2:
function testWithdrawFeesFails() public {
address[] memory players = new address[](6);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
players[4] = playerFive;
players[5] = playerSix;
puppyRaffle.enterRaffle{value: entranceFee * 6}(players);
console.log("Executing Select Winner with 6 players");
console.log("Length of the players List: ", puppyRaffle.getPlayersLength());
console.log("Contract Eth Balance: ", address(puppyRaffle).balance);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
console.log("Alice decides to exit");
vm.prank(playerOne);
puppyRaffle.refund(0);
console.log("Length of the players List: ", puppyRaffle.getPlayersLength());
console.log("Contract Eth Balance: ", address(puppyRaffle).balance);
puppyRaffle.selectWinner();
console.log("Contract Eth after selecting winner: ", address(puppyRaffle).balance);
console.log("Total fees: ", puppyRaffle.totalFees());
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
Logs
Executing Select Winner with 6 players
Length of the players List: 6
Contract Eth Balance: 6000000000000000000
Alice decides to exit
Length of the players List: 6
Contract Eth Balance: 5000000000000000000
Contract Eth after selecting winner: 200000000000000000
Total fees: 1200000000000000000
Impact
User's funds and protocol fees will be stuck in the contract
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;
}
