Wenwin contest - descharre's results

Summary

Low Risk

Non critical

Details

Low Risk

[L-01] If not enough raised, project will not work

In the unlikely scenario where the project won't sell enough tokens with their initial raise. The function finalizeInitialPotRaise() will revert and initialPot will stay 0 and this will have some consequences. This way when someone wins the jackpot they will receive a reward of 0 because the initialPot is 0.

LotterySetup.sol#L141-L143

        uint256 raised = rewardToken.balanceOf(address(this));
        if (raised < minInitialPot) {
            revert RaisedInsufficientFunds(raised);
        }

[L-02] No limit on how many tickets you can buy in one transaction

Currently there is no limit on how many tickets you can buy. buyTickets() is an expensive function. The price of one ticket isn't very high so there can always be a big player that buys a lot of tickets so he has of a bigger chance of winning the jackpot. The more nfts you buy, the more expensive the transaction gets.

When you buy around 550 tickets, which is 825 dai (usd). A big player is definitely willing to bet this kind of money. Then the function cost will be around 30 million gas which is the block gas limit. If the amount of gas exceed the gas limit, the transaction will revert and the big player will lose a lot of money in gas. Although this will not be this large if the lottery is run on the Polygon blockchain.

A mitigation is to have in if statement in buyTickets() to check that the length of tickets is not bigger than for example 500.

[L-03] No 0 check on parameters

The parameters _playerRewardFirstDraw and _playerRewardDecreasePerDraw in ReferralSystem.sol can be set to 0. In that case the playerRewards will not work and always be 0. Add a 0 check to be sure they can't be set accidently to 0.

[L-04] NetProfit is not always correct because only jackpot winners are taken into consideration

Currently the net profit is calculated in calculateReward()` with an expected payout. However if there are more winners than expected and this happens multiple times. The NetProfit can be totally wrong after a few months or years.

Non critical

[N-01] Don't use the latest solidity version

The latest version of solidity might have some bugs that we don't know about yet. Consider using 0.8.18 or 0.8.17.

Summary

Details

[G-01] Add unchecked block when operands can't under/overflow

When operands can't underflow/overflow because of a previous require() or if statement, you can use an unchecked block. The default "checked" behavior costs more gas when calculating, because under-the-hood the EVM does extra checks for over/underflow.

This can also be done for divisions.

The expression type(int).min / (-1) is the only case where division causes an overflow:

• LotterySetup.sol#L170

And when the value is only incremented by 1:

• Lottery.sol#L193-L194
• Lottery.sol#L205
• Ticket.sol#L24
• TicketUtils.sol#L70

[G-02] Use ERC721A for Ticket

Using ERC721A is a lot more gas efficiënt when you buy multiple tickets. It will be a lot of refactoring work but definitely worth it. The amount of gas that can be saved can be seen here. When you use ERC721A you basically pay the same price if you buy 1 or 5 tickets. When you buy 1 or 5 tickets with the normal ERC721 it will be a huge difference. The more tickets you buy, the more you will save. When a big player wants to buy 100 tickets at once, the gas difference will be huge.

[G-03] Remove return statements when it's never used

Both buyTickets() and registerTicket() return the TicketId but this is never used.

[G-04] Timestamps don't need to be bigger than uint48

The maximum value of uint48 is year 8 million 921 thousand 556. It will be year 36,812 when it overflows. So the struct LotteryDrawSchedule can be packed in one storage slot.

[G-05] No 0 check on claimedReward

When you call the function claimReferralReward() and the claimedReward is 0. You will still do the whole minting process of the native token even though it doesn't do anything and will mint 0 tokens.

When someone calls this function and they have 0 rewards, 10459 gas can be saved with the following if statement.

    function claimReferralReward(uint128[] memory drawIds) external override returns (uint256 claimedReward) {
        for (uint256 counter = 0; counter < drawIds.length; ++counter) {
            claimedReward += claimPerDraw(drawIds[counter]);
        }
+       if(claimedReward>0){
             mintNativeTokens(msg.sender, claimedReward);
+       }
    }

[G-06] Unnecessary storage read

The storage variable unclaimedTickets[drawId][msg.sender] gets saved twice to the same memory variable. Only referrerTicketCount gets set to 0 and it's not used afterwards so it's unneccessary to read from storage again. _unclaimedTickets can be used without updating it again.

        UnclaimedTicketsData memory _unclaimedTickets = unclaimedTickets[drawId][msg.sender];
        if (_unclaimedTickets.referrerTicketCount >= minimumEligibleReferrals[drawId]) {
            claimedReward = referrerRewardPerDrawForOneTicket[drawId] * _unclaimedTickets.referrerTicketCount;
            unclaimedTickets[drawId][msg.sender].referrerTicketCount = 0;
        }

-       _unclaimedTickets = unclaimedTickets[drawId][msg.sender];
        if (_unclaimedTickets.playerTicketCount > 0) {
            claimedReward += playerRewardsPerDrawForOneTicket[drawId] * _unclaimedTickets.playerTicketCount;
            unclaimedTickets[drawId][msg.sender].playerTicketCount = 0;
        }

[G-07] Send staking rewards to staker directly

The current implementation sends the rewardsTokens first to the stakingContract and afterwards to the staker. As you can see in getReward(), it first calls claimRewards() in Lottery.sol which sends the funds to the staking contract.

In most cases the rewardToken can be send directly to the staker. For this gas saver, I created a new function. This gas optimization works when people claim staking rewards after one or more tickets are bought. When people claim staking rewards multiple times and not tickets are bought in the meantime then there will be no gas saved. This is because Lottery.sol calculates the claimedAmount that gets send to the stakingContract with the following calculation dueTickets = nextTicketId - claimedStakingRewardAtTicketId in the function dueTicketsSoldAndReset(). So when no extra tickets are sold between 2 staking reward claims, dueTickets will be 0. When that happens we can't send the rewardToken directly to the staker but we have to send it first to the StakingContract like in the current implementation.

An example:

1. Ticket bought
2. claim staking rewards: gas saved
3. Ticket bought
4. claim staking rewards: gas saved
5. claim staking rewards: no gas saved

21129 gas is saved when reward is sent directly to the staker.

I believe more tickets will be bought than the amount of times people will claim there staking rewards, so the gas optimization will have a big effect.

I made a new function similar to claimRewards() so it has no effect on the frontend rewards:

    function claimRewardsStaking(address receiver, uint256 reward) external returns (bool directTransfer) {
        //check if msg.sender is Staking contract
        require(msg.sender == stakingRewardRecipient);
        LotteryRewardType rewardType = LotteryRewardType.STAKING;
        uint256 dueTickets = dueTicketsSoldAndReset(msg.sender);

        //if dueTickets is 0, claimedAmount will also be 0 and will transfer 0 tokens. Avoid this by returning false;
        if (dueTickets == 0) {
            return false;
        }
        uint256 claimedAmount = LotteryMath.calculateRewards(ticketPrice, dueTickets, rewardType);

        //When claimAmount is not equal to reward we have to send to the Staking first so it can transfer the tokens to the staker afterwards.
        if (claimedAmount != reward) {
            receiver = msg.sender;
        } else {
            //else we return true so the staking contract knows funds are transferred to the staker directly
            directTransfer = true;
        }

        emit ClaimedRewards(msg.sender, claimedAmount, rewardType);
        rewardToken.safeTransfer(receiver, claimedAmount);
    }

getReward() can be changed to:

    function getReward() public override {
        _updateReward(msg.sender);
        uint256 reward = rewards[msg.sender];
        if (reward > 0) {
            rewards[msg.sender] = 0;
            
            bool directTransfer = lottery.claimRewardsStaking(msg.sender, reward);
            // if reward is not transferred directly, we send the reward now
            if (!directTransfer) {
                rewardsToken.safeTransfer(msg.sender, reward);
            }

            emit RewardPaid(msg.sender, reward);
        }
    }

[G-08] Check if dueTickets is 0

If the optimization above is too complicated or the project believes there will be a lot of claiming staking rewards when no extra tickets are sold. Then there can be an if check in function claimRewards to check the return of dueTicketsSoldAndReset(beneficiary) and see if dueTickets is 0. Because now, even if there are no extra tickets sold, which means 0 reward tokens will be send to the stakingRewardRecipient, it still does the whole process of calculating the reward and transferring 0 tokens.

The best way is to return 0 so it doesn't calculate the reward and send 0 tokens. If dueTickets is 0, claimedAmount will also be 0.

    function claimRewards(LotteryRewardType rewardType) external override returns (uint256 claimedAmount) {
        address beneficiary = (rewardType == LotteryRewardType.FRONTEND) ? msg.sender : stakingRewardRecipient;
+       uint256 dueTickets = dueTicketsSoldAndReset(beneficiary);
+       if(dueTickets==0) return 0;
-       claimedAmount = LotteryMath.calculateRewards(ticketPrice, dueTicketsSoldAndReset(beneficiary), rewardType);
+       claimedAmount = LotteryMath.calculateRewards(ticketPrice, dueTickets, rewardType);
        emit ClaimedRewards(beneficiary, claimedAmount, rewardType);
        rewardToken.safeTransfer(beneficiary, claimedAmount);
    }