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

selectWinner/refund logic can result in loss of funds

equious

Summary

Blank spaces in the players array after refunds are not accounted for in the selectWinner function. The result of this is one of two things:

  • address(0) is selected as a winner and the prizePool/NFT is burned.

  • a remaining player is selected as a winner, but the prizePool is not representative of the entrants of the raffle any longer. The transfer of the prizePool will either fail, if funds are not available on the contract, or succeed, if funds are available, but because of player refunds, this will be taking the prizePool from other aspects of the protocol - effectively stealing funds.

Vulnerability Details

function testWinnersAfterRefunds() public playerEntered {
uint256 balanceBefore = address(playerOne).balance;
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(playerOne);
uint256 indexOfPlayerTwo = puppyRaffle.getActivePlayerIndex(playerTwo);
uint256 indexOfPlayerThree = puppyRaffle.getActivePlayerIndex(playerThree);
uint256 indexOfPlayerFour = puppyRaffle.getActivePlayerIndex(playerFour);
vm.deal(address(puppyRaffle), 10 ether);
​
// 3/4 players refund
vm.prank(playerOne);
puppyRaffle.refund(indexOfPlayer);
vm.prank(playerTwo);
puppyRaffle.refund(indexOfPlayerTwo);
vm.prank(playerThree);
puppyRaffle.refund(indexOfPlayerThree);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
console.log(address(playerFour).balance);
puppyRaffle.selectWinner();
console.log(address(playerFour).balance);
console.log(puppyRaffle.previousWinner());
}

The above test shows that, despite the refunds in the raffle, the winner is paid entranceFee * 4 - fees. The additional prizePool is being pulled from balances that remained on the contract.

Similarly the below test shows that selectWinner will attempt to transfer/mint to address(0), resulting in failure.

function testWinnersAfterRefunds() public playerEntered {
uint256 balanceBefore = address(playerOne).balance;
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(playerOne);
uint256 indexOfPlayerTwo = puppyRaffle.getActivePlayerIndex(playerTwo);
uint256 indexOfPlayerThree = puppyRaffle.getActivePlayerIndex(playerThree);
uint256 indexOfPlayerFour = puppyRaffle.getActivePlayerIndex(playerFour);
vm.deal(address(puppyRaffle), 10 ether);
​
// 3/4 players refund
vm.prank(playerOne);
puppyRaffle.refund(indexOfPlayer);
vm.prank(playerTwo);
puppyRaffle.refund(indexOfPlayerTwo);
vm.prank(playerFour);
puppyRaffle.refund(indexOfPlayerFour);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
}
Failing tests:
Encountered 1 failing test in test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[FAIL. Reason: ERC721: mint to the zero address] testWinnersAfterRefunds() (gas: 348525)

Impact

Burned or lost protocol funds. Any balance remaining on the contract is at risk to being paid out to raffles which do not account for refunds when selecting a winner.

Tools Used

  • Foundry

  • Manual Review

Recommendations

Assure that refunded users are properly removed from the active players array. Since totalAmountCollected and prizePool are derivative of players.length, this should resolve these potential errors.

Updates

Lead Judging Commences

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