Does mitigation of DoS in Section 4 Lesson 15 work correctly?

[setouchi-h]

Hello, I am now working on Section 4 Lesson 15 and mitigating PuppyRaffle::enterRaffle function.

I think there's a potential issue with the 2nd recommended mitigation of DoS as follows.

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;
    }
    for (uint256 i = 0; i < newPlayers.length ; i++) {
        require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");
    }
}

The issue here is that the duplicate check will always fail. The reason is that for each new player newPlayers[i], we set addressToRaffleId[newPlayers[i]] to raffleId. Therefore, the condition addressToRaffleId[newPlayers[i]] != raffleId will never be true for new players, making the duplicate check ineffective.

Is my idea correct? I'd appreciate any thoughts or feedback on this propose.

[PatrickAlphaC]

Great shout! That was sort of a quick example of what one could do, but yes you're right, this would be bad. We should remove it from the recommendations section.

[setouchi-h]

Thank you very much for your reply!!

[MohanKalai8]

Instead you can use this check

    function enterRaffle(address[] memory newPlayers){
        ...
        for (uint256 i = 0; i < newPlayers.length; i++) {
            players.push(newPlayers[i]);

            require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");

            addressToRaffleId[newPlayers[i]] = raffleId;
        }
       ...
    }

[setouchi-h]

Thank you very much!!
Your code helpes me a lot!

[AnouarBF]

No this one cannot work either, cause that will assign each player, the same raffleId, that's why we need to increase the raffleId so we can check if the player has his own id, if not, then he cannot enter the raffle.
I run it with the forge test command and it fails.
I recommend this code here:

        for (uint256 i = 0; i < newPlayers.length; i++) {
            // Check for duplicate by ensuring that the new player has an Id of 0, if not than he has already entered!
            require(
                addressToRaffleId[newPlayers[i]] == 0,
                "PuppyRaffle: Duplicate player"
            );
            players.push(newPlayers[i]);
            // increase the raffleId so the new player has the new unique id, then assign it to the player
            raffleId = raffleId + 1;
            addressToRaffleId[newPlayers[i]] = raffleId;
        }

and I removed this from the PuppyRaffle::selectWinner function, cause its useless basically:

     function selectWinner() external {
-        raffleId = raffleId + 1;

If you run the test now, it may work. And also the DoS test that we've written will fail, which means that we might have solved the DoS attack, with this new function.

Please check and reply, cause I still have doubt about it !!

[maxweisspoker]

@AnouarBF I believe you are mistaken about the purpose of the raffleId variable. It is supposed to be unique per raffle, not per player. We can have a mapping of address A->raffleId to determine whether or not A has already entered the current raffle. It is perfectly fine for B->raffleId with the same raffleId. The raffleId variable is only to ensure that the same address does not enter the same raffle twice. That is why it is incremented in the selectWinner() function, because that's when the raffle is ended and a new raffle begins.

I am watching the course series on the website and came here hoping to be the first to have spotted that the proposed fix doesn't work, and in fact creates a new DoS, preventing any new players from entering the raffle. It was fun catching the video creator make a new vulnerability with the fix, and a good example of just how that type of thing can happen, both from the auditor and from the proposal/customer when they try to mitigate a finding.