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

DoS caused by lack of active participants check

luka

Summary

No additional users can enter the raffle if at least 2 participants get refunded because of duplicate checks.

Vulnerability Details

PuppyRaffle::refund() changes the user address to address(0).
If two or more users call PuppyRaffle::refund() this will cause multiple address(0) entries in PuppyRaffle::players array and all subsequent PuppyRaffle:enterRaffle function calls will to fail due to the duplicate check.

Impact

When there are two or more inactive participants, no further entries can be made into the raffle.

If, in addition to this, there are two inactive participants and the total number of participants is less than four it becomes impossible to select a winner.

Moreover, if there is a balance in the contract, withdrawing fees is also impossible.

As a result, the PuppyRaffle::enterRaffle and PuppyRaffle::selectWinner functions are rendered non-functional.

function testDoS() public {
address[] memory playersOneTwoThree = new address[](3);
address[] memory playersFour = new address[](1);
playersOneTwoThree[0] = playerOne;
playersOneTwoThree[1] = playerTwo;
playersOneTwoThree[2] = playerThree;
playersFour[0] = playerFour;
// Players One, Two and Three enter the raffle
puppyRaffle.enterRaffle{value: entranceFee * 3}(playersOneTwoThree);
// Players One and Two get refunded
// Both of their adresses are chaged to address(0)
vm.prank(playerOne);
puppyRaffle.refund(0);
vm.prank(playerTwo);
puppyRaffle.refund(1);
// Nobody can enter the raffle because of duplicate check
// Duplicate check does not account for the inactive users -> users with address(0)
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(playersFour);
​
// Because nobody can enter the raffle and there are less then 4 active participants no winner can be selected
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
vm.expectRevert("PuppyRaffle: Need at least 4 players");
puppyRaffle.selectWinner();
}

Tools Used

Foundry

Recommendations

Recommendation one:
fix the implementation of the PuppyRaffle::_isActivePlayer function and use it when doing duplicate player checks.
See the reported vulnerability titled: "Unused and bad implementation of PuppyRaffle::_isActivePlayer function" for details of PuppyRaffle::_isActivePlayer function implementation.

function _isActivePlayer(address player) internal view returns (bool) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player && players[i] != address(0)){
return true;
}
}
return false;
}
// Check for duplicates
for (uint256 i = 0; i < players.length - 1; i++) {
+ if(_isActivePlayer(players[i])){
for (uint256 j = i + 1; j < players.length; j++) {
require(players[i] != players[j], "PuppyRaffle: Duplicate player");
}
+ }
}

Recommendation two (better option): instead of storing participants in an array store them in mapping where player address can be marked as active or inactive mapping(address => bool) public players;. This will also save gas when checking for duplicates.

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 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.

Give us feedback!