In Section 4 Lesson 15, there is an error in the suggested code for detecting duplicate users during the repair process

[kildren-coder]

Discussed in https://github.com/Cyfrin/security-and-auditing-full-course-s23/discussions/198

^{Originally posted by kildren-coder June 18, 2024} At the end of the video, we need to provide suggestions for detecting duplicate users. The approach in the video is correct, but the code provided contains errors.

The code is as follows:

+    mapping(address => uint256) public addressToRaffleId;
+    uint256 public raffleId = 0;
    .
    .
    .
    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]);
+            addressToRaffleId[newPlayers[i]] = raffleId;            
        }

-        // Check for duplicates
+       // Check for duplicates only from the new players
+       for (uint256 i = 0; i < newPlayers.length; i++) {
+          require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");
+       }    
-        for (uint256 i = 0; i < players.length; i++) {
-            for (uint256 j = i + 1; j < players.length; j++) {
-                require(players[i] != players[j], "PuppyRaffle: Duplicate player");
-            }
-        }
        emit RaffleEnter(newPlayers);
    }
.
.
.
    function selectWinner() external {
+       raffleId = raffleId + 1;
        require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");

According to the given code, all players are checked after being added to the array and addressToRaffleId[newPlayers[i]] = raffleId is set. Then, checking require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player") obviously leads to a situation where the require check cannot pass.

We expect that before a player is added to the queue, their raffleId should be checked to see if it matches the raffleId of the current round. If it matches, it means they have already joined this round of the raffle and should not be added again.

Considering the impact of default values, the correct code should be as follows:

+    mapping(address => uint256) public addressToRaffleId;
+    uint256 public raffleId = 1; // raffleId = 0 will lead to fail to add new player
    .
    .
    .
    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++) {
+            require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");            
               players.push(newPlayers[i]);
+            addressToRaffleId[newPlayers[i]] = raffleId;            
        }

-        // Check for duplicates
-        for (uint256 i = 0; i < players.length; i++) {
-            for (uint256 j = i + 1; j < players.length; j++) {
-                require(players[i] != players[j], "PuppyRaffle: Duplicate player");
-            }
-        }
        emit RaffleEnter(newPlayers);
    }
.
.
.
    function selectWinner() external {
+       raffleId = raffleId + 1;
        require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
```</div>