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

enterRaffle does not follow checks, effects, interactions

happyformerlawyer

Summary

The check for duplicate address should occur before the array of addresses is pushed to the players array. Functions should try to follow the checks, effects, interactions rule where first you do checks, then you make state changes, then you send funds, etc. In this case, you are placing a check after you have already potentially pushed duplicate players to the players array. This is low risk in this case because the function will revert if the duplicates check is failed.

Vulnerability Details

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

Impact

Given that the function reverts if the duplicate players check is failed, this is fairly low risk, but at the very least you are spending gas to push people to the array and then revert if the duplicate check is failed.

Tools Used

Manual review

Recommendations

You are forced to use this approach because of the method you are using to check duplicates. But if you used a mapping to check for duplicates (as I suggest in a different finding), you would be able to do the check before you push the address to the array and then you would be able to follow checks. effects, interactions.

Updates

Lead Judging Commences

Hamiltonite Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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