Expected payout should not be determined off-chain

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/LotterySetup.sol#L54-L59 https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/LotteryMath.sol#L84

Vulnerability details

Impact

Loss of profit and depletion of funds, the latter of which implies that some tickets will be unclaimable.

Proof of Concept

There is no guarantee to the correctness of LotterySetupParams.expectedPayout, which can be set freely (within bounds) in LotterySetup.sol:

if (
    lotterySetupParams.expectedPayout < lotterySetupParams.ticketPrice / 100
        || lotterySetupParams.expectedPayout >= lotterySetupParams.ticketPrice
) {
    revert InvalidExpectedPayout();
}

The value of expectedPayout determines the multiplier to be used when calculating non jackpot rewards (LotteryMath.sol#L84):

bonusMulti += (excessPot * EXCESS_BONUS_ALLOCATION) / (ticketsSold * expectedPayout);

Thus an incorrect or inaccurate expectedPayout may cause the payout to be too low or too high. Especially problematic is if the payout exceeds available funds. It may also result in an actual expected payout which is greater than the ticket price. This is by itself unprofitable, but will incite a run for tickets which is highly likely to drain the lottery. In both cases will drained funds leave tickets unclaimable.

Tools Used

Code inspection

Recommended Mitigation Steps

The expected payout is completely determined by the other parameters set during construction (selectionSize, selectionMax and fixedRewards). Its calculation is sufficiently simple. Consider calculating it in the contract to ensure that it is correct.

[thereksfour]

Submit QA as High Risk

[c4-judge]

thereksfour marked the issue as unsatisfactory: Overinflated severity

[d3e4]

@thereksfour Please consider the impact. This is not simply a question of facilitating parameter input, or preventing having to redeploy. The expectedPayout is critical and if it's incorrect this will not be immediately apparent, but lots of funds will be invested in the lottery which then are at risk (by the lottery's collapse). Consider especially "It may also result in an actual expected payout which is greater than the ticket price.".

[thereksfour]

In general, the risks caused by the parameters determined in the constructor will be considered as QA.

LOTTERY_EXPECTED_PAYOUT=380000000000000000 # Expected payout for one ticket sold (e.g. 0.38 in token with 18 decimals)

Also, please check the backstage guidelines.

[d3e4]

@thereksfour I can see the argument that it should be considered QA, but I think this should at least be brought to the attention of the sponsor (I assume an unsatisfactory issue otherwise might not?).

The line you quote actually demonstrates that the risk is already instantiated. 0.38 is about 2% off from the true value. This will cause a systematic error in the estimated net profit. Fortunately, now it is greater than the true value so the error will only cause the lottery to pay out less than promised, but it will not contribute to the risk of bankrupting it. But the fact that the value has been rounded to two significant digits suggest an error of up to 0.005, perhaps even 0.01 if the value is floored to two significant digits. For example, if the initial funds are one million and one million tickets are sold per draw, an error of -0.005 will cause bankruptcy in about 290 draws.

[c4-judge]

thereksfour marked the issue as nullified

[c4-judge]

thereksfour marked the issue as not nullified

[thereksfour]

@rand0c0des please take a look

[rand0c0des]

I already marked these types of issues as non issue. When deployed, if expected payout is wrong, one should just not use that deployment. However, we as a team are working on the script to automate this process.

On top of that, all the issues that claim the lottery can bankrupt are out of scope as mentioned in readme. There is certain probability of 3 out of 1000 deployments, and we are fine with that.

I would say this is not an issue at all.

[c4-judge]

thereksfour marked the issue as unsatisfactory: Invalid

[d3e4]

I already marked these types of issues as non issue. When deployed, if expected payout is wrong, one should just not use that deployment. However, we as a team are working on the script to automate this process.

That is reassuring, thanks. This is not much of an issue then.

On top of that, all the issues that claim the lottery can bankrupt are out of scope as mentioned in readme. There is certain probability of 3 out of 1000 deployments, and we are fine with that.

This regards the probability that the jackpot is won in quick succession. Actually that might not be an issue at all for some parameters. If the base jackpot is low compared to the ticket sales, then the profit from the sales in one draw will exceed the jackpot payout. I'm not sure what case 0.3% refers to. But for the issue of the risk of insolvency in cases NOT involving jackpot wins, please see my comment on #464.