Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

[M] When refunding a user, only the user's address is changed to the zero address, but the players count is not updated, which can cause the zero address to appear as a winner in a draw or cause incorrect fund calculations.

huicanvie

Root + Impact

Description

  • When a user receives a normal refund and withdraws from the lottery, it will not affect the normal lottery process and prize distribution.

  • After a user refunds, their entry in the players array is changed to the zero address; however, the zero address still participates in the lottery. If the zero address is drawn, minting the NFT fails and totalAmountCollected is computed incorrectly.

// Root cause in the codebase with @> marks to highlight the relevant section
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); //@> should delete this address
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:

  • When a user performs a normal refund


Impact:

  • If the winner is the zero address, this selectWinner method will revert immediately.

  • Because users who have already been refunded were not removed, the number of players is higher than it actually is. Therefore, when executing totalAmountCollected = players.length * entranceFee, totalAmountCollected will be larger than the actual amount, the prize will increase without cause, and the contract balance will be insufficient.

Proof of Concept

  1. Initialize by adding four users to participate in the lottery.

  2. `uint256(keccak256(abi.encodePacked(caller, block.timestamp, block.difficulty))) % 4`，calculate the winnerIndex.

  3. The user of winnerIndex refund.

  4. Then call the selectWinner

  5. revert

  6. test_Poc_WinnerIsZeroAddressTotalFeesError test the winner is normal address

  7. then revert by `OutOfFunds`


//1. if the winner is address(0)
function test_Poc_WinnerIsZeroAddress() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// 1. Fix the caller, pre-calculate which index will be selected by selectWinner
address caller = address(this);
uint256 winnerIndex = uint256(keccak256(abi.encodePacked(caller, block.timestamp, block.difficulty))) % 4;
console.log("winnerIndex: ", winnerIndex);
​
// 2. Let the player at that index refund, so that position becomes address(0)
address playerToRefund = puppyRaffle.players(winnerIndex);
vm.prank(playerToRefund);
puppyRaffle.refund(winnerIndex);
​
// 3. Use the same caller to call selectWinner, if address(0) wins, the whole transaction reverts
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
​
}
​
// 2. if the winner is normal address
function test_Poc_WinnerIsZeroAddressTotalFeesError() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// 1. Fix the caller, pre-calculate which index will be selected by selectWinner
address caller = address(this);
uint256 winnerIndex = uint256(keccak256(abi.encodePacked(caller, block.timestamp, block.difficulty))) % 4;
console.log("winnerIndex: ", winnerIndex);
​
// 2. Let the player at not winnerIndex refund, so that the winner position is normal, but the totalFees calculation is wrong, leading to funds loss
uint256 nonWinnerIndex = (winnerIndex + 1) % 4;
console.log("nonWinnerIndex: ", nonWinnerIndex);
address playerToRefund = puppyRaffle.players(nonWinnerIndex);
vm.prank(playerToRefund);
puppyRaffle.refund(nonWinnerIndex);
// 3. Use the same caller to call selectWinner, the winner is normal, but the totalFees is calculated based on 4 players instead of 3, leading to 20% of the prize pool being locked in the contract and not withdrawable by feeAddress
uint256 expectedTotalFees = (entranceFee * 3 * 20) / 100;
uint256 actualBalance = address(puppyRaffle).balance;
uint256 expectedPrizePool = (entranceFee * 4 * 80) / 100;
console.log("Expected total fees: ", uint256(expectedTotalFees));
console.log("Actual balance before: ", uint256(actualBalance));
console.log("Actual prize pool before: ", uint256(expectedPrizePool));
assertGt(expectedPrizePool, actualBalance);
// actualBalance < expectedPrizePool ,so revert
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}

Recommended Mitigation

We remove users who have been refunded by directly deleting their entries from the array.

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

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-04] `PuppyRaffle::refund` replaces an index with address(0) which can cause the function `PuppyRaffle::selectWinner` to always revert

## Description `PuppyRaffle::refund` is supposed to refund a player and remove him from the current players. But instead, it replaces his index value with address(0) which is considered a valid value by solidity. This can cause a lot issues because the players array length is unchanged and address(0) is now considered a player. ## Vulnerability Details ```javascript players[playerIndex] = address(0); @> uint256 totalAmountCollected = players.length * entranceFee; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); _safeMint(winner, tokenId); ``` If a player refunds his position, the function `PuppyRaffle::selectWinner` will always revert. Because more than likely the following call will not work because the `prizePool` is based on a amount calculated by considering that that no player has refunded his position and exit the lottery. And it will try to send more tokens that what the contract has : ```javascript uint256 totalAmountCollected = players.length * entranceFee; uint256 prizePool = (totalAmountCollected * 80) / 100; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); ``` However, even if this calls passes for some reason (maby there are more native tokens that what the players have sent or because of the 80% ...). The call will thankfully still fail because of the following line is minting to the zero address is not allowed. ```javascript _safeMint(winner, tokenId); ``` ## Impact The lottery is stoped, any call to the function `PuppyRaffle::selectWinner`will revert. There is no actual loss of funds for users as they can always refund and get their tokens back. However, the protocol is shut down and will lose all it's customers. A core functionality is exposed. Impact is high ### Proof of concept To execute this test : forge test --mt testWinnerSelectionRevertsAfterExit -vvvv ```javascript function testWinnerSelectionRevertsAfterExit() public playersEntered { vm.warp(block.timestamp + duration + 1); vm.roll(block.number + 1); // There are four winners. Winner is last slot vm.prank(playerFour); puppyRaffle.refund(3); // reverts because out of Funds vm.expectRevert(); puppyRaffle.selectWinner(); vm.deal(address(puppyRaffle), 10 ether); vm.expectRevert("ERC721: mint to the zero address"); puppyRaffle.selectWinner(); } ``` ## Recommendations Delete the player index that has refunded. ```diff - players[playerIndex] = address(0); + players[playerIndex] = players[players.length - 1]; + players.pop() ```

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!