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

Mismatched ETH Accounting in selectWinner() — Refunded Players Still Counted in Prize/Fee Calculation

quanghoa27061998

Root + Impact

Description

  • selectWinner() calculates the total prize pool and fees using players.length, which includes refunded players whose slots are address(0):

uint256 totalAmountCollected = players.length * entranceFee; // @audit Counts refunded players!
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);

  • When a player calls refund(), they receive their entranceFee back, but their slot in the players array is only set to address(0) — the array length stays the same.

  • This means totalAmountCollected is inflated beyond the actual ETH held by the contract, creating two failure modes:

    1. selectWinner() reverts — If enough players refund, prizePool exceeds the contract balance, causing winner.call{value: prizePool} to fail

    2. Phantom fees — If prizePool barely fits within balance, totalFees is incremented by fee that has no ETH backing, permanently locking withdrawFees()

Risk

Likelihood:

  • Any raffle round where at least one player refunds triggers the accounting mismatch

  • With many participants, even a single refund creates phantom fee debt

Impact:

  • High — selectWinner() can permanently revert, locking ALL remaining player funds in the contract

  • Even when selectWinner() succeeds, the fee accounting is wrong: totalFees records fees that don't exist as ETH

  • withdrawFees() becomes permanently DOSed because address(this).balance != uint256(totalFees)

Proof of Concept

How the attack works:

  1. 5 players enter the raffle, each paying 1 ETH entrance fee (contract balance = 5 ETH)

  2. 1 player calls refund() and receives 1 ETH back (contract balance = 4 ETH, but players.length is still 5)

  3. The raffle duration expires and someone calls selectWinner()

  4. totalAmountCollected = 5 * 1 ETH = 5 ETH (wrong — only 4 ETH in contract)

  5. prizePool = 4 ETH (80% of 5 ETH), fee = 1 ETH (20% of 5 ETH)

  6. winner.call{value: 4 ETH} succeeds (4 ETH available), leaving 0 ETH in contract

  7. totalFees += uint64(1 ETH) — but there is 0 ETH left to back this fee!

  8. withdrawFees() requires address(this).balance == uint256(totalFees) → 0 != 1 ETH → permanently reverts

Catastrophic scenario — 2+ refunds:

  1. 5 players enter (5 ETH), 2 refund (balance = 3 ETH)

  2. totalAmountCollected = 5 ETH, prizePool = 4 ETH

  3. winner.call{value: 4 ETH} — only 3 ETH available → REVERTS

  4. selectWinner() is permanently DOSed — no winner can ever be selected

  5. Remaining 3 players' funds are locked forever (no refund mechanism after duration expires either, as refund still works but doesn't fix the accounting)

PoC code:

function testExploit_MismatchedEthAccounting() public {
// 5 players enter
address[] memory enterPlayers = new address[](5);
enterPlayers[0] = playerOne; enterPlayers[1] = playerTwo;
enterPlayers[2] = playerThree; enterPlayers[3] = playerFour;
enterPlayers[4] = address(55);
puppyRaffle.enterRaffle{value: entranceFee * 5}(enterPlayers);
​
// 2 players refund
vm.prank(playerOne);
puppyRaffle.refund(0);
vm.prank(playerTwo);
puppyRaffle.refund(1);
​
// Contract has 3 ETH but players.length = 5
assertEq(address(puppyRaffle).balance, entranceFee * 3);
​
// Fast forward past raffle duration
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// selectWinner() tries to send prizePool = 80% of 5 ETH = 4 ETH
// But only 3 ETH in contract → REVERTS
// Try multiple msg.senders to find one that picks a non-zero winner
bool reverted = true;
for (uint256 i = 1; i <= 100; i++) {
address caller = address(uint160(i + 5000));
uint256 winnerIndex = uint256(
keccak256(abi.encodePacked(caller, block.timestamp, block.difficulty))
) % 5;
// Skip if winner would be address(0) (refunded slot)
if (winnerIndex == 0 || winnerIndex == 1) continue;
​
vm.prank(caller);
try puppyRaffle.selectWinner() {
reverted = false;
break;
} catch {
reverted = true;
break; // Confirmed: reverts due to insufficient balance
}
}
assertTrue(reverted, "selectWinner should revert — insufficient ETH for inflated prizePool");
}
​
// forge test --match-test testExploit_MismatchedEthAccounting -vvv

Expected outcome: selectWinner() permanently reverts because it tries to send more ETH than the contract holds, locking all remaining player funds. Even with only 1 refund, the fee accounting breaks and withdrawFees() becomes permanently DOSed.

Recommended Mitigation

The root cause is that selectWinner() uses players.length (which includes refunded address(0) slots) to calculate ETH amounts, while the actual contract balance reflects only the non-refunded entries. The fix must ensure the prize/fee calculation uses only the ETH that is actually held by the contract.

Primary fix — Track active player count for accurate accounting:

uint256 public activePlayerCount;
​
function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
}
activePlayerCount += newPlayers.length;
// ... duplicate check ...
emit RaffleEnter(newPlayers);
}
​
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");
​
// Effects before interactions (CEI pattern — also fixes H-001)
players[playerIndex] = address(0);
activePlayerCount--;
emit RaffleRefunded(playerAddress);
​
payable(msg.sender).sendValue(entranceFee);
}
​
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(activePlayerCount >= 4, "PuppyRaffle: Need at least 4 active players");
​
// Use activePlayerCount instead of players.length
uint256 totalAmountCollected = activePlayerCount * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + fee; // Also use uint256 per H-003
​
// ... winner selection, prize distribution ...
}

Why this works:

  • activePlayerCount precisely tracks the number of entries whose entrance fee is still held by the contract. It increments on entry and decrements on refund, so activePlayerCount * entranceFee always equals the ETH available for distribution.

  • The require(activePlayerCount >= 4) check replaces the implicit assumption that players.length >= 4, preventing selectWinner() from running when too many players have refunded.

  • prizePool and fee are now calculated from the actual ETH held, eliminating both the revert scenario (insufficient balance) and the phantom fee scenario (fees recorded without backing ETH).

Additional hardening: Consider resetting the raffle state (clearing the players array and activePlayerCount) at the end of selectWinner() to prevent stale state from carrying into future rounds:

// At the end of selectWinner():
delete players;
activePlayerCount = 0;
raffleStartTime = block.timestamp; // Start new round
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 8 days 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!