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

`refund` function breaks the contract in multiple places.

inallhonesty

Summary

The way the refund function deals with players that have solicited a refund breaks the following :

  • falsely passes the "PuppyRaffle: Need at least 4 players" require in selectWinner()
    -duplicate entrants are not allowed invariant

  • generates the "PuppyRaffle: Failed to send prize pool to winner" revert message in selectWinner()

  • generates the "ERC721: mint to the zero address" revert message in selectWinner():_safeMint

Vulnerability Details

The root of the problem is this line:

players[playerIndex] = address(0);

By replacing the address of the registered player with address(0) in the players array it gives the false impression that there are at least 4 participants, when 3 of them could have refunded their entry tickets. Leaving the number of participants = 1.

Paste the following test in PuppyRaffleTest.t.sol:

function testRefundBreaksContract() public playersEntered {
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 15);
​
vm.startPrank(playerTwo);
puppyRaffle.refund(1);
vm.stopPrank();
​
vm.startPrank(playerThree);
puppyRaffle.refund(2);
vm.stopPrank();
​
vm.startPrank(playerFour);
puppyRaffle.refund(3);
vm.stopPrank();
for(uint256 i = 0; i < 4; i++) {
address registeredPlayer = puppyRaffle.players(i);
console.log(registeredPlayer);
}

forge test --mt testRefundBreaksContract -vv

Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] testRefundBreaksContract() (gas: 209799)
Logs:
0x0000000000000000000000000000000000000001
0x0000000000000000000000000000000000000000
0x0000000000000000000000000000000000000000
0x0000000000000000000000000000000000000000

Moreover we can clearly see there are duplicates in the players array.

Add the following line to the test:

puppyRaffle.selectWinner();

Using the same terminal command we are hit by [FAIL. Reason: PuppyRaffle: Failed to send prize pool to winner]
This happens because of the way selectWinner() tracks the prizePool:

function selectWinner() external {
...
uint256 totalAmountCollected = players.length * entranceFee; //@audit-issue not representative for the actual total amount collected
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
...
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");

Given that the players.length is not changed when a player refunds his ticket, then the totalAmountCollected is not an accurate representation for the balance of the contract (the totalAmountCollected in this case is 1 * entranceFee, but the function calculates it as 4 * entranceFee). Our test fails because the contract doesn't have enough funds to send to the winner.

For testing purposes let's say the contract has some previous balance, consisting of fees that the owner of the protocol didn't withdraw.

Replace the puppyRaffle.selectWinner(); line with the following lines in our test:

vm.deal(address(puppyRaffle), 10 ether);
puppyRaffle.selectWinner();

We again run it with forge test --mt testRefundBreaksContract -vv

Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[FAIL. Reason: ERC721: mint to the zero address] testRefundBreaksContract() (gas: 334825)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.75ms

The winner of the raffle was player index 3, which in our case is address(0).

The error originates in the fact that OZ's ERC721 calls _mint inside _safeMint:

function _mint(address to, uint256 tokenId) internal virtual {
require(to != address(0), "ERC721: mint to the zero address"); //<-- Fails here
require(!_exists(tokenId), "ERC721: token already minted");
​
_beforeTokenTransfer(address(0), to, tokenId);
​
_holderTokens[to].add(tokenId);
​
_tokenOwners.set(tokenId, to);
​
emit Transfer(address(0), to, tokenId);
}

Impact

  • duplicate entrants are not allowed is broken.

  • The raffle runs with less than 4 real players.

  • In case selectWinner doesn't fail because of the way it tracks totalAmountCollected protocol's owner funds will be used to pay the prize.

  • Most likely the selectWinner function will fail because you can't mint an NFT to address 0 while using OZ's ERC721 implementation.

Tools Used

Manual review, forge testing

Recommendations

Update the refund function as follows:

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);
​
++ payable(msg.sender).sendValue(entranceFee);
++ // Shift elements to the left starting from playerIndex
++ for (uint256 i = playerIndex; i < players.length - 1; i++) {
++ players[i] = players[i + 1];
++ }
++ // Remove the last element (duplicated due to shifting)
++ players.pop();
​
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!