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

(H-3) Not accounting for players that get refunds leads to wrong calculations, higher fee and winner payout costs and failed payout transactions

zxarcs

Summary

Players that got refunds are not considered when calculating prize pool and fee pools. This leads those calculations to be bloated and when the contract tries to send out the funds, the transaction will fail as not enough funds are available.

Details

When selecting a winner, in selectWinner function, the code does not take into consideration users that might of got a refund. totalAmountCollected is calculated by multiplying the players array length and the entranceFee
$$ totalAmountCollected = players.length * entranceFee $$

When a player gets a refund, their address in the players array gets changed to the zero address and their entrance fee gets returned to them. This is not taken into consideration in the selectWinner function.

Filename

src/PuppyRaffle.sol

Permalinks

https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/07399f4d02520a2abf6f462c024842e495ca82e4/src/PuppyRaffle.sol#L131-L133

Impact

Wrong calculations for:

  • totalAmountCollected

  • prizePool

  • fee and totalFees

Contract will try to send more funds that it has and those transactions will fail.

Recommendations

In addition to checking that there is at least 4 players for the raffle, It is also recommended to make sure that the total players does not include any zero addresses which represents players that got refunds. A new array could be created with this count and only the non zero addresses should be populated to the new array. Then safely loop over that array.

Instead of using players.length to get total valid players use something similar to below. Which only counts the non-zero-address players.

uint256 validPlayers;
for (uint256 i = 0; i < players.length; i++) {
if (players[i] != address(0)) {
validPlayers++;
}
}

Tools Used

  • Manual Audit

  • Foundry

POC

address[] memory refflePlayers = new address[](4);
refflePlayers[0] = address(8);
refflePlayers[1] = address(9);
refflePlayers[2] = address(0); // this player got a refund and should not be counted in total players count
refflePlayers[3] = address(10);
uint256 validPlayers;
for (uint256 i = 0; i < refflePlayers.length; i++) {
if (refflePlayers[i] != address(0)) {
validPlayers++;
}
}
console.log("valid players", validPlayers); // 3
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.