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

Incorrect `players.length` in `PuppyRaffle::refund` Leads Excess Withdraws

krisrenzo

Summary

The selectWinner function relies on the players.length value to calculate totalAmountCollected. However, players.length can be inaccurate as it never decreases.

Vulnerability Details

When players get refunded, their address is simply replaced with an address(0), causing players.length to remain the same. This affects the calculation of totalAmountCollected in the selectWinner function, which is used to calculate the winner's prize and the protocol's fee.

function refund(uint256 playerIndex) public {
//...
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Proof of Concept

The provided test suite demonstrates the validity and severity of this vulnerability.

How to Run the Test

Requirements:

  • Install Foundry.

  • Clone the project codebase into your local workspace.

Step-by-step Guide to Run the Test:

  1. Ensure the above requirements are met.

  2. Copy the test below and add it to PuppyRaffleTest.t.sol tests.

  3. Execute the following command in your terminal to run the test:

forge test --match-test "testAccountingLogic"

Code

function testAccountingLogic() public playersEntered {
/// @notice playerOne gets a refund
vm.prank(playerOne);
puppyRaffle.refund(0);
​
/// @notice playerTwo gets a refund
vm.prank(playerTwo);
puppyRaffle.refund(1);
​
/// @notice playerThree gets a refund
vm.prank(playerThree);
puppyRaffle.refund(2);
​
/// @notice raffle ends and the winner is selected
vm.prank(address(this));
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
vm.expectRevert("PuppyRaffle: Failed to send the prize pool to the winner");
puppyRaffle.selectWinner();
}
​

Impact

Implications:
If there are more than three players and everyone gets refunds, selectWinner is still callable because the players.length doesn't change. Also, there's a high probability of the PuppyRaffle balance not being able to cover the prize pool paid to the winner or the fee sent to the protocol, causing selectWinner to revert. Lastly, the more players that get refunded, the higher the chances of the contract selecting address(0) as the winner, which leads to loss of funds.

Tools Used

  • Foundry

Recommendations

Instead of replacing the refunded player with address(0), move the last player in the players array to the refunded player's position and then pop the array. This is what your function will look like:

function refund(uint256 playerIndex) public {
// ...
+ players[playerIndex] = players[players.length-1];
+ players.pop();
+ payable(msg.sender).sendValue(entranceFee);
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

Hamiltonite Lead Judge about 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.

Give us feedback!