Beginner FriendlyFoundryNFT
100 EXP
View results
Submission Details
Severity: high
Valid

`refund()` sets refunded players to `address(0)` leading to duplicates in `players` and preventing others from calling `enterRaffle()`

Summary

The refund() function sets refunded entrants to address(0) leading to duplicates in the players array and therefore preventing others from joining after either the refund() function is called at least twice or when address(0) is entered and refund() is called once.

Vulnerability Details

Since the protocol is not allowing duplicates in the enterRaffle function and sets refunded entrants to address(0) in the refund function, this can lead to address(0) showing up as duplicates when the refund() function was at least called twice or when someone enters address(0) and calls refund() with another entrant once.

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++) {
players.push(newPlayers[i]);
}
@> // Check for duplicates
@> 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);
}
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);
}

Impact

Change the code in PuppyRaffleTest.t.sol and run forge test --mt testMultipleRefundsCorruptEnterRaffle and forge test --mt testZeroAddressCorruptsEnterRaffleAfterRefund to see how calling the refund() function twice or entering address(0) and calling the refund() function prevents new players from entering.

+ modifier playersAndZeroAddressEntered() {
+ address[] memory players = new address[](4);
+ players[0] = playerOne;
+ players[1] = playerTwo;
+ players[2] = playerThree;
+ players[3] = address(0);
+ puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
+ _;
+ }
+ function testZeroAddressCorruptsEnterRaffleAfterRefund() public playersAndZeroAddressEntered {
+ vm.prank(playerOne);
+ puppyRaffle.refund(0);
+ address[] memory player = new address[](1);
+ player[0] = playerFour;
+ vm.expectRevert();
+ puppyRaffle.enterRaffle{value: entranceFee}(player);
+ }
+ function testMultipleRefundsCorruptEnterRaffle() public playersEntered {
+ vm.prank(playerOne);
+ puppyRaffle.refund(0);
+ vm.prank(playerTwo);
+ puppyRaffle.refund(1);
+ address[] memory player = new address[](1);
+ player[0] = address(69);
+ vm.expectRevert();
+ puppyRaffle.enterRaffle{value: entranceFee}(player);
+ }

Tools Used

  • Foundry

Recommendations

Instead of setting a refunded entrant to address(0), delete the address in the players array by setting it to the last member and deleting it.

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

patrickalphac Lead Judge
over 1 year ago
Hamiltonite Lead Judge over 1 year 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.