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

Refunded slots are still counted in selectWinner, so the pool is overstated and the raffle gets stuck or fees get locked

ward0x

Root + Impact

Description

  • After a refund, the raffle should only count and pay out the funds that are actually still in it.

  • refund only overwrites the slot with address(0); players.length does not shrink. selectWinner computes totalAmountCollected = players.length * entranceFee, so every refunded ticket is counted again. The prize + fee is then larger than the balance. With 4 entered and 1 refunded the 3.2 ETH prize cannot be paid from 3 ETH and every selectWinner reverts (the 4-player check also passes with only 3 real players). With 5 entered and 1 refunded the draw succeeds, but totalFees records 1 ETH while the contract holds 0, so withdrawFees can never pass its equality check. The winner can also be a zero slot (mint to zero reverts).

function refund(uint256 playerIndex) public {
...
@> players[playerIndex] = address(0); // slot kept, length unchanged
}
​
function selectWinner() external {
@> require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
...
@> uint256 totalAmountCollected = players.length * entranceFee; // counts refunded slots
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);

Risk

Likelihood:

  • Refunds are an advertised feature; a single refund in a round is enough.

Impact:

  • The raffle gets stuck (no winner can ever be drawn, remaining players' funds are locked) or the fee accounting is permanently broken so the owner's fees can never be withdrawn.

Proof of Concept

All tests run in a test contract that deploys PuppyRaffle(1e18, address(99), 1 days) and uses the helpers _players(start,n) (distinct addresses) and _enter(p) (pays 1e18 * p.length). Full file: test/AuditPoC.t.sol, forge test --mc AuditPoC passes.

function test_H3_refundedSlotsOverstatePool_raffleStuck() public {
address[] memory p = _players(1, 4);
_enter(p);
vm.prank(p[0]);
r.refund(0);
vm.warp(block.timestamp + 1 days);
for (uint256 i; i < 20; i++) {
vm.warp(block.timestamp + 1);
vm.expectRevert();
r.selectWinner(); // needs 3.2 ETH, holds 3 ETH
}
assertEq(address(r).balance, 3 * fee);
}
​
function test_H3b_refundedSlotsLockFees() public {
address[] memory p = _players(1, 5);
_enter(p);
vm.prank(p[1]);
r.refund(1);
vm.warp(block.timestamp + 1 days);
bool ok;
for (uint256 i; i < 50 && !ok; i++) {
vm.warp(block.timestamp + 1);
(ok,) = address(r).call(abi.encodeWithSignature("selectWinner()"));
}
assertTrue(ok);
assertEq(address(r).balance, 0);
assertEq(uint256(r.totalFees()), fee); // 20% of 5 ETH recorded, 0 held
vm.expectRevert("PuppyRaffle: There are currently players active!");
r.withdrawFees();
}

Recommended Mitigation

Track the number of active players / collected value instead of players.length, or remove the refunded player from the array (swap with last and pop):

- players[playerIndex] = address(0);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();

and compute totalAmountCollected from the active count only.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 9 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!