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

`players.length` remain unchanged after calling `refund()` which may cause loss of funds to address(0) and create calculation errors in `selectWinner()`.

naman1729

Summary

  • players.length remain unchanged after calling refund() which may cause loss of funds to address(0) and create calculation errors in selectWinner().

Vulnerability Details

Click this to see Code
/// @param playerIndex the index of the player to refund. You can find it externally by calling `getActivePlayerIndex`
/// @dev This function will allow there to be blank spots in the array
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);
}

  • Here, the refund() function is called to refund the player at playerIndex and set the player's address to address(0). However, the players.length is not changed, which means that the players array will have a address(0) at playerIndex . This may cause loss of funds to address(0) if selectWinner() is selected as the winner to playerIndex.

  • Also, this may cause calculation errors in selectWinner() . Given in blow Code.

Click this to see Code
/// @notice this function will select a winner and mint a puppy
/// @notice there must be at least 4 players, and the duration has occurred
/// @notice the previous winner is stored in the previousWinner variable
/// @dev we use a hash of on-chain data to generate the random numbers
/// @dev we reset the active players array after the winner is selected
/// @dev we send 80% of the funds to the winner, the other 20% goes to the feeAddress
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();
​
// We use a different RNG calculate from the winnerIndex to determine rarity
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 above code, players.length is used to calculate totalAmountCollected. If players.length is not changed, totalAmountCollected will be calculated incorrectly. for example, initially players.length is 5, and player call refund function to refund the player at playerIndex 2, then players.length is still 5, but the players array will have a address(0) at playerIndex 2. So, totalAmountCollected = 5 * entranceFee, but it should be equal to totalAmountCollected = 4 * entranceFee.

  • Pass the require check, for example, players.length is 4, and player call refund function to refund the player at playerIndex 2, then players.length is still 4, but the players array will have a address(0) at playerIndex 2. So, players.length >= 4, but the actual number of players is 3.

  • Calculate winnerIndex is uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length; , so if players.length is not changed, the winnerIndex will be calculated incorrectly. for example, initially players.length is 5, and player call refund function to refund the player at playerIndex 2, then players.length is still 5, but the players array will have a address(0) at playerIndex 2. So, winnerIndex = uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % 5; , but it should be equal to winnerIndex = uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % 4;. Also, winnerIndex may be equal to 2, which is the address(0) in our exmaple. This may cause loss of funds to address(0).

Impact

  • Loss of funds to address(0) and create calculation errors in selectWinner().

Tools Used

  • Manual Review

  • Remix

Recommendations

/// @param playerIndex the index of the player to refund. You can find it externally by calling `getActivePlayerIndex`
/// @dev This function will allow there to be blank spots in the array
function refund(uint256 playerIndex) public {
+ require(playerIndex < players.length, "PuppyRaffle: Player index out of bounds");
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] = players[players.length-1];
+ players.pop();
​
payable(msg.sender).sendValue(entranceFee);
​
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
​

  • Add require(playerIndex < players.length, "PuppyRaffle: Player index out of bounds"); to check whether the playerIndex is out of bounds.

  • Change players[playerIndex] = address(0); to players[playerIndex] = players[players.length-1]; and players.pop(); to delete the player at playerIndex and change the length of players array.

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years 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.

Give us feedback!