code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/LotteryMath.sol#L52-L53 https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L240 https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L209 https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L212

Vulnerability details

Impact

There is a wrong calculation of the cumulative net profit of the lottery, which affects the calculation of the excess pot and rewards per winning ticket (including the jackpot) in each draw. This vulnerability also leads to a Denial of Service of the Lottery contract.

Proof of Concept

The result of LotteryMath.calculateMultiplier should be used to calculate a percentage of the expected payout for all tickets. This should be done using the getPercentage method from the PercentageMath library, as is done in LotteryMath.calculateReward:

  96:    function calculateReward(
            ...
 107:    {
 108:        uint256 excess = calculateExcessPot(netProfit, fixedJackpot);
 109:        reward = isJackpot
 110:            ? fixedReward + excess.getPercentage(EXCESS_BONUS_ALLOCATION) // @audit <- PoC
 111:            : fixedReward.getPercentage(calculateMultiplier(excess, ticketsSold, expectedPayout));
 112:    }

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/LotteryMath.sol#L96-L112

The problem is that instead of calculating the expectedRewardsOut in LotteryMath.calculateNewProfit as a percentage from ticketsSold * expectedPayout, the current implementation will multiply the result of the multiplication by the returned basis points from calculateMultiplier. This will result in making the newProfit extremely high and therefore impact the calculation of rewards and excess pot in all future draws. This can also lead to a DoS because the calculated profit will always be extremely lowered each time the jackpot is not won (since expectedRewardsOut will grow exponentially due to the excess pot, and the multiplier will grow as well) and therefore come to an underflow situation after a certain time has passed.

Tools Used

Manual Review

Recommended Mitigation Steps

Fix the code in LotteryMath.calculateNewProfit in the following way:

  50:    uint256 expectedRewardsOut = jackpotWon
  51:        ? calculateReward(oldProfit, fixedJackpotSize, fixedJackpotSize, ticketsSold, true, expectedPayout)
- 52:        : calculateMultiplier(calculateExcessPot(oldProfit, fixedJackpotSize), ticketsSold, expectedPayout)
- 53:            * ticketsSold * expectedPayout;
+ 52:        : (ticketsSold * expectedPayout).getPercentage(
+ 53:           calculateMultiplier(
+ 54:               calculateExcessPot(oldProfit, fixedJackpotSize),
+ 55:               ticketsSold,
+ 56:               expectedPayout
+ 57:           )
+ 58:       );

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/LotteryMath.sol#L50-L53

c4-judge commented 1 year ago

thereksfour marked the issue as duplicate of #219