Seasons

In the uploadEntrants function if the first user in the provided users array is being processed then the previousTickets amount used to write to the cumulativeTickets array is 0. This will be inaccurate if the uploadEntrants function is called multiple times, which it is likely to be in the event that a large number of entrants must be uploaded. Additionally when processing subsequent users the cumulativeTickets value is read using the current iteration index - 1, which would also yield an inaccurate previousTickets value when the uploadEntrants function is called multiple times. This will perturb the ability for users with higher ticket IDs to win the season as the random value is modulo by a totalTickets amount which will be smaller than the actual total tickets.

Instead of basing the previousTickets on the current users array index, base it off of the length of the cumulativeTickets array length + the current user array index.

A winner should only be able to claim once. I have added a patch in the linked commit

**The **selectWinner function implements a validation against a winner being selected when the desired amountOfWinners has already been met, however since the winner is not pushed to the winners array until fulfillment of the random request the admin could intentionally or accidentally call the selectWinner function more than the amountOfWinners number of times and allow the system to accrue more than the amountOfWinners in the winners array.

Consider also including the winners length validation in the _fulfillRandomness function.

In the initSeason function the price ERC20 tokens are transferred in using transferFrom without validating the return value. Instead safeTransfer should be used to safely handle any arbitrary price tokens. Additionally in the claimPrizes function transfer is used without checking the return value when sending ERC20 tokens to the winner.

Use safeTransferFrom for the transferring of ERC20 prize tokens in the initSeason function and safeTransfer when sending ERC20 prize rewards to the winner in the claimPrizes function.

In the Season contract there is no logic in the claimPrizes function for the PrizeType.OFFCHAIN prize type. This may be the expected behavior and all logic might be computed off-chain, however it may be useful to emit an event in the claimPrizes function to indicate that off-chain rewards should be delivered, for instance it may be useful to see which off-chain item uuid should be received by the user.

Consider whether the claimPrizes function should include any logic, such as an event to indicate that off chain prizes have been claimed.

The LiquidityLaunchFactory contract allows users to deploy a custom token including the ability to set a name and symbol for the new token. However, it is possible for an attacker to craft a name such that it includes markup that can contain Javascript code. If loaded into a frontend without XSS protection, this can cause potential harm to users of the platform as was the case in the EtherDelta exploit. For more details on the EtherDelta exploit please refer to this article: https://medium.com/@zhongqiangc/code-injection-attacks-using-blockchains-part-1-db4e121890bb

Consider adding a length limit to the token name in addition to the pre-existing validation for the symbol.

In the claimPrizes function there is no requirement that all winners have been chosen when the claim is taking place. This may be unexpected for the Zap team.

Consider whether all winners should be chosen before any claims are allowed to occur, and if so then implement the corresponding validation in the claimPrizes function.

In the claimPrizes function when awarding a winner with an ERC721 token they have won safeTransferFrom is used which will revert if the receiver is a smart contract and does not implement the onErc721Received function. For participants that use a smart contract address this will prevent them from claiming all of their rewards.

Be sure to document this risk to users so this issue does not arise.

In the comment for the onERC721Received function the word receiver is misspelled as reciever.

Correct the spelling to receiver.

In the Season contract the uploadEntrants function can be called after winners have already been chosen. This action would not normally be taken by an admin, however it should be explicitly prevented to avoid any wasted gas if the transaction were to be accidentally sent.

Add validation to the uploadEntrants function that asserts that no winners have been chosen.

The isValidRequestHash variable is currently hardcoded to true with the comment that This is a temporary fix to make the test pass.

Be sure to resolve this comment and use the actual logic for the isValidRequestHash value.

In the fulfillRandomness function there is an if case which relies on the isValidRequestHash value after the isValidRequestHash value has already been required to be true.

Either remove the require statement or the if case as both are unnecessary together.

In the event that one or just a couple users have purchased the majority of tickets in a season it may be a very high probability that a previous winner is selected in the _fulfillRandomness Function. If the odds are sufficiently large for a small group of winners then it may be unrealistic to be able to complete a Season. Additionally this will cost an unnecessary amount of gas fees to the operator and additional fees to cover the Gelato cost.

Be aware of this risk and ensure Seasons have a wide distribution of users with no single large ticket holders. Furthermore if this behavior should be avoided, consider adding an escape hatch to end a Season early or even removing winners from the entrantAddresses and cumulativeTickets, though this will likely introduce more complexity than the resolution is worth.

In the _requestRandomness function the requestedHash entry for the request is not cleared. This diverts from the latest GelatoVRFConsumerBase contract implementation here: https://github.com/gelatodigital/vrf-contracts/blob/main/contracts/GelatoVRFConsumerBase.sol

Consider adding logic to delete the requestedHash in the event that the random number was accepted.

The randomTicketNumber value is computed using modulo for the final randomized value, however this can lead to slightly unfair probability distributions due to what is known as modulo bias. For example, consider the simplified scenario where the totalTickets is 3 and the random number is generated from 0 to 10 inclusive. There are three outcomes for the randomTicketNumber, however the probabilities which are shown below are not equal. <table> <tr> <td>Random</td> <td>Result Mod 3</td> </tr> <tr> <td>0</td> <td>0</td> </tr> <tr> <td>1</td> <td>1</td> </tr> <tr> <td>2</td> <td>2</td> </tr> <tr> <td>3</td> <td>0</td> </tr> <tr> <td>4</td> <td>1</td> </tr> <tr> <td>5</td> <td>2</td> </tr> <tr> <td>6</td> <td>0</td> </tr> <tr> <td>7</td> <td>1</td> </tr> <tr> <td>8</td> <td>2</td> </tr> <tr> <td>9</td> <td>0</td> </tr> <tr> <td>10</td> <td>1</td> </tr> </table>

Probabilities for randomTicketNumber: <table> <tr> <td>randomTicketNumber</td> <td>Probability</td> </tr> <tr> <td>0</td> <td>4/11 = 36.36%</td> </tr> <tr> <td>1</td> <td>4/11 = 36.36%</td> </tr> <tr> <td>2</td> <td>3/11 = 27.27%</td> </tr> </table> In actuality the totalTickets will be much larger and the random number is generated in the entire range of a uint256 word, however this simplified example demonstrates how the probabilities may not be even amongst all ticket numbers when the modulo does not divide type(uint256).max + 1.

In actuality the modulo bias experienced by Zap will be reduced notably as higher ticket amounts are minted, so it is likely that this bias can be safely ignored for the purposes of Zap’s Season contract. However if modulo bias should be a priority to remove, one approach could be to require that tickets purchased always result in a modulo divisor that divides the cardinality of possible random numbers, e.g. type(uint256).max + 1 (all positive integers in a uint256 and 0).

**In the **uploadEntrants function there is no validation that the provided user.tickets value is greater than 0. If a user.tickets value of 0 were to be used then a user can exist in the entrantAddresses array multiple times which would be unexpected behavior.

The uploadEntrants function is performed by a trusted party, however to avoid any unintentional mistakes it may be prudent to add validation which prevents a user.tickets value of 0.

In the LiquidityFreeLaunch contract the zapDegenTokenReciever variable misspells receiver as reciever.

Correct the spelling to receiver.

In the seedWithFeeAndAcceptToken function the feeForZapTreasury is bought with the getTokensOutForEthDeposit and the results are added to the totalSoldand balance values before the initial seed amount is added. This results in the fee being larger and the seeded amountOut being smaller. Using explicative numbers, if the feeForZapTreasury is 0.01 ETH and the seedAmount is 0.1 ETH then:

For the feeForZapTreasury: A = (1 + k * 0.01)^n = 1.00593 startSupply = 0 endSupply = totalTokens * (1 - 1/A) = 910_374_363 * (1 - 1/1.00593) = 5_366_695 amountOutForZapTreasury = 5_366_695 tokens

For the amountOut: A = (1 + k * (0.1 + 0.01))^n = 1.06696 startSupply = 5_366_695 tokens endSupply = totalTokens * (1 - 1/A) = 910_374_363 * (1 - 1/1.06696) = 57_133_039 amountOut = 57_133_039 - 5_366_695 = 51_766_344 tokens

However if the ordering were reversed the amountOut is larger and the zap treasury fee is smaller.

For the amountOut: A = (1 + k * (0.1))^n = 1.06072 startSupply = 0 tokens endSupply = totalTokens * (1 - 1/A) = 910_374_363 * (1 - 1/1.06072) = 52_113_593 amountOut = 52_113_593 tokens

For the feeForZapTreasury: A = (1 + k * (0.1 + 0.01))^n = 1.06696 startSupply = 52_113_593 tokens endSupply = totalTokens * (1 - 1/A) = 910_374_363 * (1 - 1/1.06696) = 57_133_039 amountOutForZapTreasury = 57_133_039-52_113_593 = 5_019_446 tokens

This may be the expected behavior since the fee can be reduced for larger initial seed amounts, however it should be confirmed whether this ordering is expected and clearly documented for users.

In the LiquidityFreeLaunch contract the DUST_THRESHOLD constant is unused.

Remove or implement the use case for the DUST_THRESHOLD.

In the _fulfillRandomness function a while(true) loops is now used to re-draw random numbers to avoid selecting a winner that has already been chosen. In cases where a single user has the majority of tickets in a raffle this loop may execute many times before finding a unique winner, and may even need more gas than the block gas limit. This will incur a large gas fee and thus a large gelato fee to Zap and may not result in a unique winner being chosen. To limit the gas expenditure for cases where the loop will execute many times a while loop iteration limit can be introduced.

Consider introducing an iteration limit in the _fulfillRandomness function to ensure that unexpectedly large gas fees are not levied on the protocol.