code-423n4 / 2023-03-wenwin-findings

1 stars 1 forks source link

Estimated profit may drift #464

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L40 https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/LotteryMath.sol#L35-L56

Vulnerability details

Impact

The lottery is not sustainable over time. Excess pot calculation may drift, which either leads to insufficient payouts or a depletion of funds.

Proof of Concept

The payouts are calculated using an estimate of the net profit, currentNetProfit. There is limited tolerance for errors in the estimated net profit: if the estimate is too low the lottery will payout too little (leaving funds effectively stuck in the contract), and if it is too high the contract may run out of funds.

currentNetProfit is updated (by calculateNewProfit) each draw by adding an estimate of the payout. The average error in the payout does indeed tend towards zero, but the cumulative error does NOT. The error sources are both the intrinsic randomness as well as unclaimed tickets. This means that currentNetProfit may over time drift arbitrarily far from its true value. There is currently nothing that corrects for this.

Tools Used

Code inspection

Recommended Mitigation Steps

There must be a correction of currentNetProfit towards its true value. Since tickets may remain unclaimed for up to a year it is only possible to directly calculate the net profit until a year in the past. This may be sufficient to contain the drift. Intermediate estimations may also be used, on the assumption that most tickets are claimed much sooner than within a year.

c4-judge commented 1 year ago

thereksfour marked the issue as unsatisfactory: Insufficient proof

d3e4 commented 1 year ago

@thereksfour Please reconsider this issue and let the sponsor have a look as well. The drift in currentNetProfit follows from its being a sum of estimations, the cumulative error of which has no bound. This is just statistical common knowledge. The mean of the error should be zero, but it necessarily has some (independent) variance, and variance is additive. So the random walk that is the cumulative sum of errors will reach arbitrarily high and low given enough time. How fast this happens depends on the parameters and ticket sales, but eventually it almost surely will.

The above is sufficient to conclude that there is an issue, but let's quantify two examples to illustrate the severity. With the parameters given in the docs, and a base jackpot of 10,000, if 50,000 tickets are sold each draw, then the probability that the lottery's balance goes to zero is 21% within 500 draws. It can be even worse if a small volume follows a long period of high volume. For example, if the base jackpot is 50,000 and one million tickets are sold each draw for 400 draws, and the volume then drops to 50,000, then there is a 30% risk that the balance will be negative, and it will most likely be stuck at negative until the volume goes up again, meaning that tickets will remain unclaimable.

thereksfour commented 1 year ago

Will reopen it for sponsors, @rand0c0des please take a look

c4-judge commented 1 year ago

thereksfour marked the issue as nullified

c4-judge commented 1 year ago

thereksfour marked the issue as not nullified

rand0c0des commented 1 year ago

This is not an issue, as it is out of scope.

rand0c0des commented 1 year ago

All the probabilities are calculated carefully, and we clearly stated what is the probability of lottery being insolvent + mentioned this type of issue is out of scope.

c4-judge commented 1 year ago

thereksfour marked the issue as unsatisfactory: Invalid

d3e4 commented 1 year ago

@rand0c0des @thereksfour I'm sorry to push on this, but please not that this issue does NOT depend on the jackpot being won more or less than expected. It doesn't depend on anything unexpected. Nor does it depend on incorrect probabilities. I'm aware of the "There is a certain scenario when the lottery would run out of funds. This can happen in extreme scenarios when the jackpot is won in consecutive draws, while the ticket sales were low. The probability of this happening is 0.3%. This issue will not be considered valid.". It is indeed quite robust against jackpot wins (the accounting in that case is exact). But this is a completely different issue. Maybe the current risk of insolvency is a risk you accept but it can be far greater than 0.3%, especially if the lottery turns out to be small. If the base jackpot and weekly sales are 1000 then the probability is 63%. (I have also confirmed this by a simulation.)

thereksfour commented 1 year ago

As a judge, I would consider it an oos issue because the sponsor clearly stated "there is a certain scenario when the lottery would run out of funds". Even if it was valid, including it would be unfair to the other warden.

d3e4 commented 1 year ago

The only thing that is the same here is the impact of running out of funds, not the mechanism for how it happens. My argument is that the already known "certain scenario" is explicitly specified by "extreme scenarios when the jackpot is won in consecutive draws, while the ticket sales were low" which is not at all the scenario I describe nor does it share the same root cause. I just want to make sure that the sponsor is aware of this.

rand0c0des commented 1 year ago

Of course we are aware. I just listed the one that is the easiest to prove.