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

Denial of service in `PuppyRaffle::enterRaffle` if >= 2 refunds have occurred

ciara

When a player calls PuppyRaffle::refund, the player's address is replaced with address(0). In enterRaffle(), there is a check on players as to whether a duplicate address exists and reverts if there is. If 2 or more players have refunded, address(0) will exist multiple times in the players array. Subsequent calls to enterRaffle() will erroneously revert due to the duplicated refunded addresses, even if the array of address(s) provided to enterRaffle() contain no duplicates. This is a denial of service (DoS) on entering the raffle.

Vulnerability Details

In PuppyRaffle:enterRaffle, when player(s) enters a raffle there is a check that no duplicate addresses exist on line 86-90:

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

However, when a player calls refund(), their address is replaced with address(0) in the players array, as seen on line 103:

players[playerIndex] = address(0);

If >= 2 players call refund() then address(0) will exist in the array more than once and so enterRaffle() will revert on every call resulting in a state of DoS.

Impact

DoS on calling enterRaffle() means that attackers can increase their chance of winning by not allowing other players to enter the raffle.

Since address(0) can win the raffle (see issue: "Zero address can win the lottery if a player has called PuppyRaffle::refund") there is a situation in which no one can win the raffle if all active players call refund() meaning that no other players can enter. selectWinner() can still be called but address(0) has 100% chance of winning since it occupies every seat in the raffle. This will result in a DoS as selectWinner() will always revert as address(0) cannot receive NFTs.

Since players being able to enter the raffle is a core capability of a raffle, hence, this is a high-severity vulnerability.

Proof of Concept

Working Test Case

The following test enters two players into the raffle. Both players call refund() and then both players attempt to enter the raffle again. If this test passes, enterRaffle() will revert due to a duplicated address:

function test_poc_PlayerCantEnterIf2OrMoreRefunds() public {
address[] memory players = new address[](2);
players[0] = playerThree;
players[1] = playerTwo;
puppyRaffle.enterRaffle{value: entranceFee * 2}(players);
vm.prank(playerThree);
puppyRaffle.refund(0);
vm.prank(playerTwo);
puppyRaffle.refund(1);
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee * 2}(players);
}

Running the test yields the following output, demonstrating that the test passes and players can no longer enter the raffle:

forge test --mt test_poc_PlayerCantEnterIf2OrMoreRefunds
​
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_PlayerCantEnterIf2OrMoreRefunds() (gas: 178644)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.70ms
​
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

To mitigate this issue, include a second array that contains a mapping of address to boolean determining whether the player is active or not and set this boolean when calling refund() rather than setting the refunding address to address(0):

+ mapping(address->bool) activityStatus;
​
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(activityStatus(newPlayers[i]) == false, "Player already exists)
activityStatus[newPlayers[i]] = true;
players.push(newPlayers[i]);
}
​
- 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);
}

To refund, the activityStatus will be set to false for the refunding address.

Since getActivePlayerIndex() is only used externally so that the value can be used to call refund, getActivePlayerIndex() can be removed and refund modified to take an address:

+ function refund(address playeraddress) public {
- address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
+ require(activityStatus[playerAddress] == true, "PuppyRaffle: Player already refunded, or is not active");
- players[playerIndex] = address(0);
+ activityStatus[playerAddress] = false;
+ payable(msg.sender).sendValue(entranceFee);
​
emit RaffleRefunded(playerAddress);
}

When drawing the winner, the players activity status can be checked, and only allow active players to be drawn as the winner.

Tools Used

Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

denial-of-service-in-enter-raffle

Support

FAQs

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