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

DoS on calling `PuppyRaffle::selectWinner` if all players refund

ciara

When a player calls PuppyRaffle::refund, if successful, the player's address is replaced with address(0) in the player array. This means that address(0) now has a seat in the raffle and therefore a non-zero chance of winning. If all players in the raffle refund, then address(0) has a 100% chance of winning. Since address(0) cannot receive ERC-721 tokens, selectWinner() will revert, this will result in a permanent state of DoS since new players, who can receive NFTs, can call enterRaffle() due to the duplicated address(0) players array element (see issue: "Denial of service in PuppyRaffle::enterRaffle if >= 2 refunds have occurred" for more details)

Vulnerability Details

In PuppyRffle::refund, on line 103, the refunding address is replaced with address(0) in the players array when a player refunds:

players[playerIndex] = address(0);

When selectWinner() is subsequently called, there is a chance that none of the players will win as address(0) can now win the lottery. If all players call refund(), address(0) has a 100% chance of winning. Since address(0) cannot receive ERC-721 tokens, selectWinner() will revert (see issue: "Deterministic value for the rarity of the winner's NFT due to bad randomness in PuppyRaffle::selectWinner") resulting in a permanent state of DoS

Impact

address(0) can win the raffle. There is also a situation in which no one can win the raffle since four players enter, all four call refund() meaning that no other players can enter (see issue "Denial of service in PuppyRaffle::enterRaffle if > 2 refunds have occurred"). selectWinner() can still be called but address(0) has 100% chance of winning since it occupies every seat in the raffle and this will result in a DoS as address(0) cannot receive funds. This is a permanent state of DoS since address(0) will win every time but selectWinner() will always revert. No one else can enter the raffle, no one can ever win, meaning that the contract can no longer function. This is, therefore, a high-severity vulnerability as the likelihood of all players refunding at any point during the lottery, especially at the start, is high.

Proof of Concept

Working Test Case

The following test demonstrates that if four players enter the raffle and all four call refund(), the winner of the raffle will always be address(0), enterRaffle() will revert, and selectWinner() will revert:

function test_poc_zeroAddressWinLottery() public playersEntered {
address[] memory players = new address[](2);
players[0] = playerThree;
players[1] = playerTwo;
for (uint256 i = 0; i < 4; i++) {
address player = address(i+1);
vm.prank(player);
puppyRaffle.refund(i);
}
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// DoS selecting winner
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
​
// DoS entering raffle
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee * 2}(players);
}

As expected, the test passes, demonstrating that the contract can no longer function as expected and enterRaffle() and selectWinner() are in a permanent state of DoS:

$ forge test --mt test_poc_zeroAddressWinLottery -vvvv
​
// output
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_zeroAddressWinLottery() (gas: 337416)
Traces:
[341628] PuppyRaffleTest::test_poc_zeroAddressWinLottery()
├─ [121273] PuppyRaffle::enterRaffle{value: 4000000000000000000}([0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000004])
│ ├─ emit RaffleEnter(newPlayers: [0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000004])
│ └─ ← ()
├─ [0] VM::prank(0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [29749] PuppyRaffle::refund(0)
│ ├─ [3000] PRECOMPILE::ecrecover{value: 1000000000000000000}()
│ │ └─ ←
│ ├─ emit RaffleRefunded(player: 0x0000000000000000000000000000000000000001)
│ └─ ← ()
├─ [0] VM::prank(0x0000000000000000000000000000000000000002)
│ └─ ← ()
├─ [27458] PuppyRaffle::refund(1)
│ ├─ [60] PRECOMPILE::sha256{value: 1000000000000000000}()
│ │ └─ ← 0xe3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
│ ├─ emit RaffleRefunded(player: 0x0000000000000000000000000000000000000002)
│ └─ ← ()
├─ [0] VM::prank(0x0000000000000000000000000000000000000003)
│ └─ ← ()
├─ [27890] PuppyRaffle::refund(2)
│ ├─ [600] PRECOMPILE::ripemd{value: 1000000000000000000}()
│ │ └─ ← 0x0000000000000000000000009c1185a5c5e9fc54612808977ee8f548b2258d31
│ ├─ emit RaffleRefunded(player: 0x0000000000000000000000000000000000000003)
│ └─ ← ()
├─ [0] VM::prank(0x0000000000000000000000000000000000000004)
│ └─ ← ()
├─ [27361] PuppyRaffle::refund(3)
│ ├─ [15] PRECOMPILE::identity{value: 1000000000000000000}()
│ │ └─ ←
│ ├─ emit RaffleRefunded(player: 0x0000000000000000000000000000000000000004)
│ └─ ← ()
├─ [0] VM::warp(86402 [8.64e4])
│ └─ ← ()
├─ [0] VM::roll(2)
│ └─ ← ()
├─ [0] VM::expectRevert(PuppyRaffle: Failed to send prize pool to winner)
│ └─ ← ()
├─ [60652] PuppyRaffle::selectWinner()
│ ├─ [0] 0x0000000000000000000000000000000000000000::fallback{value: 3200000000000000000}()
│ │ └─ ← "EvmError: OutOfFund"
│ └─ ← "PuppyRaffle: Failed to send prize pool to winner"
├─ [0] VM::expectRevert(PuppyRaffle: Duplicate player)
│ └─ ← ()
├─ [46757] PuppyRaffle::enterRaffle{value: 2000000000000000000}([0x0000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000002])
│ └─ ← "PuppyRaffle: Duplicate player"
└─ ← ()
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.39ms

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

  • Forge

Updates

Lead Judging Commences

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

refund-doesnt-reduce-players-array-size-causing-protocol-to-freeze

zero address can win the raffle

Funds are locked to no one. If someone gets the refund issue, they also got this issue. IMPACT: High Likelihood: High

Support

FAQs

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