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

If one person enters a friend into the raffle, their friend can get a refund of the money

happyformerlawyer

Summary

If one person decides to enter themselves and more than one other address (and of course pays the entry fees for those other people), a person in control of one of those other addresses can get a refund for the amount paid by the original entering person. If you enter for someone and then maybe have a falling out with them or they lose their private keys, the person entering the raffle won't be able to get a refund for that entry.

Vulnerability Details/Test

I wrote this test to demonstrate that one player can take the refund paid to enter them - playerOne enters three people and then playerTwo takes the money playerOne paid:

function testCanGetRefundExpanded() public {
address[] memory players = new address[](3);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
vm.deal(playerOne, 10 ether);
vm.deal(playerTwo, 10 ether);
vm.prank(playerOne);
puppyRaffle.enterRaffle{value: entranceFee * 3}(players);
uint256 balanceBefore = address(playerTwo).balance;
uint256 indexOfPlayer = puppyRaffle.getActivePlayerIndex(playerTwo);
vm.prank(playerTwo);
puppyRaffle.refund(indexOfPlayer);
assertEq(address(playerTwo).balance, balanceBefore + entranceFee);
}

Run this test and it will pass even though playerTwo is getting a refund for playerOne's money

Impact

Some entries are not refundable in the way you would expect. You could argue that maybe the players sent their money to entering player to enter for them. But maybe they didn't do that way and it was just a friend entering for multiple people (or even multiple entries for themself, but they might lose the key for one of the addresses). Most people would expect you could refund either all of your entry or maybe none of your entry, not part of it.

Tools Used

Foundry
VS Code

Recommendations

You can create a mapping of the address of the person who called enterRaffle to an array of the addresses they entered. Then you push all the addresses they entered to that mapping in enterRaffle and finally you put a check at the beginning of refund that the address they are trying to get a refund for is in msg.sender's array in that mapping...see below changes:

Add a mapping variable:

mapping(address => address[]) private addresstoAddressesEntered;

Add a custom error:

error PuppyRaffle__RefundOnlyForEnteringPlayer();

Add a line to the first for loop in enterRaffle to push all addresses entered to the new mapping:

addresstoAddressesEntered[msg.sender].push

Add the following function:

function _checkIfMsgSenderEnteredAddress(address playerAddress) internal returns(bool) {
for (uint256 i = 0; i < addressToAddressesEntered[msg.sender].length; i++) {
if(addressToAddressesEntered[msg.sender][i] == playerAddress) {
return true}
}
}

Add the following check to the refund function after playerAddress is defined:

if(_checkIfMsgSenderEnteredAddress(playerAddress) == false) {
revert PuppyRaffle__RefundOnlyForEnteringPlayer();}
Updates

Lead Judging Commences

patrickalphac 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.