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]);
}
@>
@> 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
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);
}