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

PRaffle Protocol Falis to select WInner Protocol

daspra0tik

Root + Impact

Description

  • In the Puppy Raffle FirstFlight 1 contract, the refund function is intended to return a user's entrance fee and remove them from the active players list. However, it only updates the player's index to address(0) rather than removing the element and shrinking the array's .length.

  • The selectWinner function calculates the totalAmountCollected (and the subsequent 80% prizePool) by multiplying the static entranceFee by players.length. If any user has claimed a refund, the actual Ether balance of the contract will be less than the calculated totalAmountCollected. When selectWinner attempts to transfer the inflated prize pool to the winner, the transaction will revert due to an EVM OutOfFunds error.


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);
}

Risk

  • Permanent Denial of Service (DoS): If even one user calls refund(), the selectWinner function becomes permanently deadlocked and will always revert.

  • Trapped Funds: Because the raffle can never be concluded, all remaining players' entrance fees, as well as the protocol's uncollected fees, are permanently locked inside the contract.

Proof of Concept

  • This test proves that a partial refund by just two players completely bricks the protocol's ability to select a winner:

  1. Arrange: A group of players enters the raffle (handled by the playersToEnterInRaffle modifier), and the contract holds their pooled entrance fees.

  2. Act: Two players (spider at index 0, and alice at index 1) call the refund() function. The contract correctly refunds their Ether, lowering the contract's actual balance, but fails to decrease the players array length.

  3. Act: Time is fast-forwarded by 2 days (vm.warp) to bypass the raffleDuration lock.

  4. Assert: spider attempts to call selectWinner(). The contract calculates a payout based on the original number of players, but because 2 players' worth of Ether has already left the contract, it cannot afford the payout. The vm.expectRevert() correctly catches the transaction crashing, proving the protocol is permanently broken.

modifier playersToEnterInRaffle() {
//Arrange
​
address[] memory the_players = new address[](5);
​
the_players[0] = spider;
the_players[1] = alice;
the_players[2] = bob;
the_players[3] = dan;
the_players[4] = eli;
​
//Act
vm.prank(spider);
pRaffle.enterRaffle{value: STARTING_AMOUNT * 5}(the_players);
_;
}
​
function test_Fails_to_selectWinner() public playersToEnterInRaffle {
//Arrange
//Act
vm.prank(spider);
pRaffle.refund(0);
vm.prank(alice);
pRaffle.refund(1);
​
uint256 balance_of_contract = address(pRaffle).balance;
console2.log("Balance of Contract: ", balance_of_contract);
​
vm.warp(2 days);
​
vm.startPrank(spider);
vm.expectRevert();
pRaffle.selectWinner();
vm.stopPrank();
​
uint256 balance_of_after_calling_selectWinner_function = address(
pRaffle
).balance;
console2.log(
"Balance of Contract: ",
balance_of_after_calling_selectWinner_function
);
​
uint256 totalFees = pRaffle.totalFees();
console2.log("Total Fees: ", totalFees);
//Assert
assertEq(
balance_of_contract,
balance_of_after_calling_selectWinner_function,
"As 2 people Refunded but the state doesnt track it"
);
}
[PASS] test_Fails_to_selectWinner() (gas: 223916)
Logs:
Balance of Contract: 3000000000000000000
Balance of Contract: 3000000000000000000
Total Fees: 0
​
Traces:
[285160] TestPRaffle::test_Fails_to_selectWinner()
├─ [0] VM::prank(spider: [0xB279F90f644e63EAca636d78E1d3fcC206632F63])
│ └─ ← [Return]
├─ [148412] PuppyRaffle::enterRaffle{value: 5000000000000000000}([0xB279F90f644e63EAca636d78E1d3fcC206632F63, 0x70E1B74cD0d17f05De348115B5Cd2772812B906F, 0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e, 0xb72116984E306d834a0ae638688Ef9AF1f7FE2cd, 0xEa61F454C2B4A5A16AB556DBE8DBB176C1D02177])
│ ├─ emit RaffleEnter(newPlayers: [0xB279F90f644e63EAca636d78E1d3fcC206632F63, 0x70E1B74cD0d17f05De348115B5Cd2772812B906F, 0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e, 0xb72116984E306d834a0ae638688Ef9AF1f7FE2cd, 0xEa61F454C2B4A5A16AB556DBE8DBB176C1D02177])
│ └─ ← [Stop]
├─ [0] VM::prank(spider: [0xB279F90f644e63EAca636d78E1d3fcC206632F63])
│ └─ ← [Return]
├─ [9281] PuppyRaffle::refund(0)
│ ├─ [0] spider::fallback{value: 1000000000000000000}()
│ │ └─ ← [Stop]
│ ├─ emit RaffleRefunded(player: spider: [0xB279F90f644e63EAca636d78E1d3fcC206632F63])
│ └─ ← [Stop]
├─ [0] VM::prank(alie: [0x70E1B74cD0d17f05De348115B5Cd2772812B906F])
│ └─ ← [Return]
├─ [9281] PuppyRaffle::refund(1)
│ ├─ [0] alie::fallback{value: 1000000000000000000}()
│ │ └─ ← [Stop]
│ ├─ emit RaffleRefunded(player: alie: [0x70E1B74cD0d17f05De348115B5Cd2772812B906F])
│ └─ ← [Stop]
├─ [0] console::log("Balance of Contract: ", 3000000000000000000 [3e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::warp(172800 [1.728e5])
│ └─ ← [Return]
├─ [0] VM::startPrank(spider: [0xB279F90f644e63EAca636d78E1d3fcC206632F63])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf4844814)
│ └─ ← [Return]
├─ [73859] PuppyRaffle::selectWinner()
│ ├─ [0] 0x0000000000000000000000000000000000000000::fallback{value: 4000000000000000000}()
│ │ └─ ← [OutOfFunds] EvmError: OutOfFunds
│ └─ ← [Revert] PuppyRaffle: Failed to send prize pool to winner
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("Balance of Contract: ", 3000000000000000000 [3e18]) [staticcall]
│ └─ ← [Stop]
├─ [2443] PuppyRaffle::totalFees() [staticcall]
│ └─ ← [Return] 0
├─ [0] console::log("Total Fees: ", 0) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.51ms (1.26ms CPU time)

Recommended Mitigation

  • The refund function must explicitly shrink the array when a user leaves the raffle. Implement the standard "Swap and Pop" pattern to keep the array length synchronized with the contract's actual balance:

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);
​
// 1. Swap the refunded player with the last player in the array
players[playerIndex] = players[players.length - 1];
// 2. Remove the last element and shrink the array length by 1
players.pop();
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

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