Beginner FriendlyFoundryNFT
100 EXP
View results
Submission Details
Severity: high
Valid

Low Risk Report

L-01. Use of players array increases the gas costs associated with entering the raffle

Summary

The players array causes the gas costs to enter the raffle to increase as more players have entered.

Vulnerability Details

A test to estimate the gas usage when calling enterRaffle for an additional player given that a given number of players had already entered the raffle was conducted. This test is shown below:

function testGasUsageEnterRaffle() public {
// Setup address array: players
address[] memory players = new address[](1);
for (uint256 i; i < players.length; ++i) {
players[i] = makeAddr(string(abi.encodePacked("CAW CAW NUMBER ", vm.toString(i))));
}
// Enter raffle with players
puppyRaffle.enterRaffle{value: players.length * entranceFee}(players);
// Enter raffle as this address
address[] memory entry = new address[](1);
entry[0] = address(this);
uint256 startGas = gasleft();
puppyRaffle.enterRaffle{value: entranceFee}(entry);
emit log_uint(startGas - gasleft());
}

Results: when 1, 10 and 100 players had already entered the raffle, the gas costs for an additional player to enter (i.e. the entry array) are 33690, 78903 and 4046058, respectively. This is due to the for loop duplicate checks.

Impact

Higher gas costs for users and potential for out-of-gas errors preventing additional players from entering.

Tools Used

Foundry.

Recommendations

  • Use a mapping to track active players (example: mapping(address => bool) playersEntered. This way you don't have to cross check the address with all other players entered, you can just track the boolean which indicates whether the address is already in the raffle. This mapping could also potentially be a bitmap which would further lower gas costs.

  • When using a mapping, change the order of the enterRaffle function such that: the address is checked to see if it has already entered the raffle and then the state change is made.

L-02. The function _isActivePlayer is redundant.

Summary

This function is an internal function and is not used anywhere within the PupyyRaffle contract.

Recommendations

This function should either be: utilised within the contract as an internal helper function, change to a public or external function for use by users or removed from the contract.

L-03. Unreliable sources of randomness are used to select winners and token rarities.

Summary

block.timestamp and block.difficulty have been used in the selectWinner function to compute winner index and the NFT token rarity to be minted.

Vulnerability Details

It is possible for an attack contract to calculate winnerIndex and rarity variables, and determine the conditions (timestamp and difficulty) necessary to win the raffle and/or achieve a particular rarity. The msg.sender and block.timestamp are predictable. While block.difficulty is relatively unpredictable, it is not completely random.

Impact

Unfair raffle results.

Recommendations

Use a provably fair and verifiable random number generator such as Chainlink VRF.

Updates

Lead Judging Commences

patrickalphac Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

weak-randomness

Root cause: bad RNG Impact: manipulate winner

denial-of-service-in-enter-raffle

Support

FAQs

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

Give us feedback!