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

Incorrect Prize Pool Calculation When Players Refund

raihanmd

Incorrect Prize Pool Calculation When Players Refund

Description

  • The raffle allows players to request a refund by calling the refund() function. When a refund occurs, the player's address in the players array is replaced with address(0).

    However, the contract does not reduce the size of the players array after a refund. Instead, it leaves an empty slot while the array length remains unchanged.

    Later, when selecting the raffle winner, the contract calculates the total collected amount using the length of the players array:

uint256 totalAmountCollected = players.length * entranceFee;

This calculation assumes that every slot in the players array represents a valid paid participant. However, refunded players remain in the array as address(0), meaning they are still counted toward the total collected amount even though their entrance fee has already been returned.

// Root cause in the codebase with @> marks
​
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
​
payable(msg.sender).sendValue(entranceFee);
​
players[playerIndex] = address(0); //@> refunded player still counted in players.length
}
uint256 totalAmountCollected = players.length * entranceFee; //@> assumes all players paid

This mismatch between the actual funds held by the contract and the calculated total collected amount can lead to incorrect prize pool distribution.

Risk

Likelihood: Medium

  • Refunds are a supported feature of the protocol.

  • Each refund creates a zero slot while players.length remains unchanged.

Impact: Medium

  • The prize pool calculation may assume more funds exist than are actually available.

  • This can lead to incorrect accounting between the winner payout and the protocol fee.

Proof of Concept

// Entrance fee = 1 ETH
// Players join = 10 players
// Total collected = 10 ETH
​
addressp[] players = new address[](10);
​
raffle.enterRaffle(players);

Two players request refunds:

// Actual contract balance = 8 ETH
// players.length = 10

When selecting a winner:

// totalAmountCollected = 10 ETH
// prizePool = 8 ETH
// fee = 2 ETH

However, the contract only holds 8 ETH, causing the accounting logic to be inconsistent with the real balance.

Recommended Mitigation

Track the number of active players instead of relying on players.length.

Alternatively, remove refunded players from the array.

- players[playerIndex] = address(0);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 6 days 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!