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

Incorrect update of the players array

jfornells

PuppyRuffle::refund loss of funds in the protocol.

Description

  • When a player calls refund(), the contract should correctly reflect that this player is no longer active, avoiding counting them in subsequent calculations.

  • However, refund() marks the player as refunded by setting players[playerIndex] = address(0), but does not remove the element or reduce players.length. This leaves "holes" in the array and causes players.length to keep counting players who no longer contribute funds.

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);
​
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood: High

  • This situation occurs every time a player calls refund, because the array always keeps the same length.

  • It manifests whenever subsequent logic uses players.length as the real number of participants (e.g. winner selection, prizePool/fees calculation or requirements).

Impact: High

  • Incorrect economic calculations: prizePool and fee are inflated compared to the real available funds.

  • Can cause reverts and functionality blocking (DoS) during selectWinner() or payments, if trying to send more ETH than available.

Proof of Concept

This test demonstrates that after a refund(), the players array is not updated correctly, as only the player's address is set to address(0) but players.length is not reduced.

function test_Refund_Do_Not_Update_Correctly() public {
// Assign 8 ETH to playerOne to enter the raffle
deal(playerOne, 8 ether);
​
// Simulate all actions as playerOne
vm.startPrank(playerOne);
​
// Create an array of 5 players
// All enter paying entranceFee
address memory newPlayers = new address[](5);
newPlayers[0] = playerOne;
newPlayers[1] = playerTwo;
newPlayers[2] = playerThree;
newPlayers[3] = playerFour;
newPlayers[4] = address(5);
​
// Enter the raffle with a total of 5 ETH
puppyRaffle.enterRaffle{value: 5 ether}(newPlayers);
​
// playerOne calls refund:
// players[0] becomes address(0) but players.length remains 5
puppyRaffle.refund(0);
​
// Advance time so the raffle can finish
vm.warp(block.timestamp + 1 days);
​
// Winner is selected using players.length (5)
// even though only 4 players have real funds
puppyRaffle.selectWinner();
​
// Fee withdrawal fails because:
// - fees were calculated with inflated players.length
// - the real contract balance is insufficient
puppyRaffle.withdrawFees();
}
​
log:
├─ [37224] PuppyRaffle::withdrawFees()
│ ├─ [0] 0x0000000000000000000000000000000000000063::fallback{value: 1000000000000000000}()
│ │ └─ ← [OutOfFunds] EvmError: OutOfFunds
│ └─ ← [Revert] PuppyRaffle: Failed to withdraw fees
└─ ← [Revert] PuppyRaffle: Failed to withdraw fees

Recommended Mitigation

Mitigation consists of keeping the players array compact using the swap and pop pattern.

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");
​
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
payable(msg.sender).sendValue(entranceFee);
​
- players[playerIndex] = address(0);
​
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 9 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!