executeDraw doesn't return non-jackpot claims to the prize pool

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L271-L277

Vulnerability details

Impact

Prize pool will be smaller than intended and will lead to excess funds that can't be accessed

Proof of Concept

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L271-L277

function returnUnclaimedJackpotToThePot() private {
    if (currentDraw >= LotteryMath.DRAWS_PER_YEAR) {
        uint128 drawId = currentDraw - LotteryMath.DRAWS_PER_YEAR;
        uint256 unclaimedJackpotTickets = unclaimedCount[drawId][winningTicket[drawId]];
        currentNetProfit += int256(unclaimedJackpotTickets * winAmount[drawId][selectionSize]);
    }
}

Lottery#returnUnclaimedJackpotToThePot only returns the jackpot winning if it hasn't been claimed after one year.

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L159-L168

function claimable(uint256 ticketId) external view override returns (uint256 claimableAmount, uint8 winTier) {
    TicketInfo memory ticketInfo = ticketsInfo[ticketId];
    if (!ticketInfo.claimed) {
        uint120 _winningTicket = winningTicket[ticketInfo.drawId];
        winTier = TicketUtils.ticketWinTier(ticketInfo.combination, _winningTicket, selectionSize, selectionMax);
        if (block.timestamp <= ticketRegistrationDeadline(ticketInfo.drawId + LotteryMath.DRAWS_PER_YEAR)) {
            claimableAmount = winAmount[ticketInfo.drawId][winTier];
        }
    }
}

The issue with this is that NO tickets can be claimed after a year, including non-jackpot tickets. The result is that after a year has passed the prize pool is no longer obligated to pay those tickets but the currentNetProfit is not updated to reflect this.

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/LotteryMath.sol#L50-L55

    uint256 expectedRewardsOut = jackpotWon
        ? calculateReward(oldProfit, fixedJackpotSize, fixedJackpotSize, ticketsSold, true, expectedPayout)
        : calculateMultiplier(calculateExcessPot(oldProfit, fixedJackpotSize), ticketsSold, expectedPayout)
            * ticketsSold * expectedPayout;

    newProfit -= int256(expectedRewardsOut);

When calculating the new profit in LotteryMath#calculateNewProfits, these expected payouts are removed from the profit of the pool. If the tickets are never claimed after a year then their value will be forever excluded. Since this variable is used to determine payouts it will forever skew the pool causing a portion of funds to be incorrectly excluded.

Tools Used

Manual Review

Recommended Mitigation Steps

The unclaimed non-jackpot rewards should be added back into the pool when the tickets expire. Implementing this is rather straight forward. Store the expected rewards for each non-jackpot drawID. Each time a ticket is claimed, remove the value of the ticket claimed from the stored rewards. After a year when Lottery#returnUnclaimedJackpotToThePot add back in the surplus (or deficit) in expected rewards that resulted from those tickets not being claimed.

[c4-judge]

thereksfour marked the issue as primary issue

[rand0c0des]

Communicated over discord that this is by design. We cannot return non jackpot rewards as we do not have exact number of winners. This is why we use estimate. Not an issue

[c4-sponsor]

rand0c0des marked the issue as sponsor disputed

[thereksfour]

Closed as a known issue

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-c