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

selectWinner sizes the pot from players.length, so refunded slots over-pay the winner out of fees and lock withdrawFees

thedelph

Summary

refund returns a player's ETH but only zeroes their slot; players.length does not change. selectWinner then computes the pot as players.length * entranceFee, which still counts every refunded slot as a paid ticket. After a single ordinary refund, the draw does one of three things:

  • pays the winner out of already-earned protocol fees, which permanently breaks withdrawFees;

  • reverts because the contract doesn't hold the computed prize;

  • lands on the zeroed slot, and _safeMint(address(0)) reverts.

Description

src/PuppyRaffle.sol:103, in refund: the slot is zeroed and the array is not shortened.

players[playerIndex] = address(0);

src/PuppyRaffle.sol:130-134, in selectWinner:

address winner = players[winnerIndex]; // can be address(0)
uint256 totalAmountCollected = players.length * entranceFee; // counts refunded slots
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);

Each refund leaves an entranceFee gap between the ETH actually held for the round and totalAmountCollected. The prize (:151) is paid from the contract's whole balance, which includes accrued totalFees. The booked fee is also inflated. From then on, address(this).balance < totalFees holds for good, and the strict equality in withdrawFees (:158) can never be satisfied again.

Risk

Likelihood: High — refunds are a normal user flow, so any round with a refund triggers it.

Impact: High

Refunding is a documented, normal user action (README rule 3), so any round with a refund triggers one of the following:

  1. Protocol fees stolen and locked. Round 1 leaves 0.8 ETH of fees. In round 2, 5 players enter and 1 refunds, so the balance is 4.8 ETH. The draw pays 4 ETH (80% of 5) instead of 3.2 ETH, taking 0.8 ETH of fees. The final balance is 0.8 ETH, but totalFees = 1.8 ETH. withdrawFees reverts forever, so every past and future fee is locked.

  2. Draw reverts. With 4 players and 1 refund, the balance is 3 ETH but the prize is 3.2 ETH, so selectWinner reverts with "Failed to send prize pool to winner".

  3. Zero-address winner. If winnerIndex hits a refunded slot, the prize call to address(0) succeeds, then _safeMint(address(0)) reverts with "ERC721: mint to the zero address".

Proof of Concept

test/PuppyRaffleAudit.t.sol:

  • test_PoC_RefundedSlotOverpaysWinnerFromFeesAndLocksWithdraw

  • test_PoC_RefundedSlotMakesDrawRevert

  • test_PoC_RefundedZeroSlotCanWinAndRevertsMint

function test_PoC_RefundedSlotOverpaysWinnerFromFeesAndLocksWithdraw() public {
// Round 1: normal, leaves 0.8 ETH of fees in the contract.
_enter(1, 4);
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
assertEq(address(puppyRaffle).balance, 0.8 ether);
​
// Round 2: 5 players, one refunds.
_enter(10000, 5);
vm.prank(address(10004));
puppyRaffle.refund(4);
assertEq(address(puppyRaffle).balance, 4.8 ether); // 4 live tickets + 0.8 fees
​
vm.warp(block.timestamp + duration + 1);
_warpUntilNotIndex(address(this), 5, 4); // winner is a live player
puppyRaffle.selectWinner();
​
assertEq(address(puppyRaffle).balance, 0.8 ether);
assertEq(uint256(puppyRaffle.totalFees()), 1.8 ether);
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
function test_PoC_RefundedSlotMakesDrawRevert() public {
_enter(1, 4);
vm.prank(address(1));
puppyRaffle.refund(0); // 3 ETH held, prize computed as 3.2 ETH
vm.warp(block.timestamp + duration + 1);
_warpUntilNotIndex(address(this), 4, 0);
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}
​
function test_PoC_RefundedZeroSlotCanWinAndRevertsMint() public {
_enter(1, 5);
vm.prank(address(5));
puppyRaffle.refund(4);
vm.warp(block.timestamp + duration + 1);
_warpUntilIndex(address(this), 5, 4); // draw lands on the zeroed slot
vm.expectRevert("ERC721: mint to the zero address");
puppyRaffle.selectWinner();
}

Run:

forge test --match-test "test_PoC_Refunded(Slot|ZeroSlot)" -vvv

The output is in poc.txt: balance 0.8e18, totalFees 1.8e18.

Recommended Mitigation

Remove refunded players from the array instead of zeroing them (swap-and-pop), so that players.length always equals the number of paid, active tickets. Alternatively, keep an explicit active-player counter and use it for the pot, and never select an empty slot.

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);
+
+ payable(msg.sender).sendValue(entranceFee);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 38 minutes 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!