Wenwin contest - Yukti_Chinta's results

Lines of code

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

Vulnerability details

When finalizing a draw, Wenwin removes the expected rewards, including bonuses, from the protocol net profit.

In a non-jackpot draw, calculateNewProfit sets these expectedRewardsOut to:

        uint256 expectedRewardsOut = calculateMultiplier(...) * ticketsSold * expectedPayout;
        newProfit -= int256(expectedRewardsOut);

calculateMultiplier returns a multiplier with PERCENTAGE_BASE decimals - eg. 110000. It is multiplied by the expectedPayout, to get the new expected payout; But since it is not divided by PERCENTAGE_BASE, the new expectedRewardsOut would grow by a factor of 100000, instead of 1-2.

Impact

The calculated protocol net profit (and balance) would be smaller by a factor of 100000 than it really is. This value is used to calculate the rewards for the next draw - which will be wrongly calculated to be tiny, or even negative. Therefore, users will not get the correct amount of rewards which they deserve, and the funds would be lost in the contract.

Proof of Concept

First of all, note that calculateMultiplier returns number with PERCENTAGE_BASE digits:

    /// @return bonusMulti Multiplier to be used when calculating rewards, with `PERCENTAGE_BASE` precision
    function calculateMultiplier(

This can also be seen in this function we created which shows the value of the multiplier for real inputs.

Let's look at a correct usage of calculateMultiplier, in calculateReward:

            fixedReward.getPercentage(calculateMultiplier(excess, ticketsSold, expectedPayout));

Here, getPercentage is used on calculateMultiplier, so it is divided by PERCENTAGE_BASE, as should be.

But this does not happen in calculateNewProfit's usage of calculateMultiplier. There, calculateMultiplier is used by itself, raw:

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

        newProfit -= int256(expectedRewardsOut);

So the idea here is to get ticketsSold * expectedPayout * bonusMultiplier, for example ticketsSold * expectedPayout * 1.1. But instead we are getting ticketsSold * expectedPayout * 110000. This huge amount is then substracted from the protocol net profit, which will impact the rewards for future rounds. (The path of usage is receiveRandomNumber calls drawRewardSize which passes the wrong currentNetProfit to calculateReward).

Therefore this will lose a huge amount of funds, when a non-jackpot round is drawn.

Recommended Mitigation Steps

Add getPercentage in calculateNewProfit: getPercentage((ticketsSold * expectedPayout), calculateMultiplier(...)).

Low Risk Issues

Summary Of Findings:

Details on Findings:

1. <ins>In the early stages users ends up receiving very little rewards because of massive locked stake from the protocol-team</ins>

During the initial token launch, out of the 1 billion tokens, 20% of it(which is 200m), is allocated to team in one go. Though this is locked for 1 year, its still staked in the Staking contract and is eligible for a share of rewards. This is fine, if during the initial sale, the rest of the 750m are bought by users and they stake it in the Staking contract as well.

But generally, in the initial stages, the number of native tokens in the hand of regular users will be much less than 200m. Which means a huge share of rewards will go to the protocol team.

So, to make it fairer to regular users, It is recommended that, the protocol team doesnt get all the 200m tokens at once. One way would be to use a simple formula, like this: Tokens allocated to team = min(200m, Tokens sold to regular users)

This will let the team grow in a linear way with the rest of the users.

2. <ins>unsafe revert in fulfillRandomWords function</ins>

fulfillRandomWords function in VRFv2RNSource.sol has an unsafe revert which can make the whole lottery system stuck. According to Chainlink security guidelines, this function must never revert, as once called it will not get called again. So a revert will totally brick the system and lead to massive loss of funds.

Reference - https://docs.chain.link/vrf/v2/security/#fulfillrandomwords-must-not-revert

Though you shouldn't receive more than one word from Chainlink, we recommended to just use the first word instead of reverting if there are multiple words.

3. <ins>Mismatch between documentation and smart contract in getMinimumEligibleReferralsFactorCalculation function</ins>

The getMinimumEligibleReferralsFactorCalculation function uses a series of if conditions to decide the minimumEligible referral amount based on which range totalTicketsSoldPrevDraw falls into. There is a small difference here, between the documentation and code.

The doc uses <= sign where code uses <. Either the doc should be corrected or the code.

4. <ins>Wrong formula for calculating Excess Pot</ins>

calculateExcessPot function in LotteryMath uses a somewhat different formula for calculating the Excess Pot than the one in the documentation does. This will result in discrepancies if not set up correctly.

    function calculateExcessPot(int256 netProfit, uint256 fixedJackpotSize) internal pure returns (uint256 excessPot) {
        int256 excessPotInt = netProfit.getPercentageInt(SAFETY_MARGIN);
        excessPotInt -= int256(fixedJackpotSize);
        excessPot = excessPotInt > 0 ? uint256(excessPotInt) : 0;
    }

In short, the formula in code boils down to:

excessPotInt = netProfit * SAFETY_MARGIN - fixedJackpotSize;

But the actual formula from the doc shows:

excessPotInt = netProfit * (1-SAFETY_MARGIN) - fixedJackpotSize;

Submitting it as a QA, only because, the developers might have stored 1-SAFETY_MARGIN as SAFETY_MARGIN in the code. Otherwise this is a med or high severity.

5. <ins>The window for claiming winning tickets closes before 1 year</ins>

The documentation says that:

Winning tickets have a one-year deadline from the draw time to claim their prize by redeeming the winning NFT ticket.

But this is not implemented in the code correctly. The time period is a bit less than 1 year. The claimable function checks if the current timestamp is less than or equal to ticketRegistrationDeadline(ticketInfo.drawId + LotteryMath.DRAWS_PER_YEAR).

The ticketRegistrationDeadline function returns firstDrawSchedule + (drawId * drawPeriod) - drawCoolDownPeriod;.

Assuming each draw lasts for 7 days and 52 draws per year, this contributes to 7*52 = 360 days. Which is at least 5 days less than an year. Further, the drawCoolDownPeriod is subtracted from the timestamp, which means deadline for claiming the winning tickets, end at least drawCoolDownPeriod before 1 year period.

It is recommended to mention this fact in the doc for transparency. Or the code could be modified from: if (block.timestamp <= ticketRegistrationDeadline(ticketInfo.drawId + LotteryMath.DRAWS_PER_YEAR)) {

to

if (block.timestamp <= drawScheduledAt(ticketInfo.drawId) + 1 year) {

6. <ins>The first draw should start only after the 200 million team tokens are staked and locked for 1 year</ins>

The documentation says 20% of the native tokens will be allocated to the team which is around 200 million in this case. These tokens are supposed to be locked for an year. But this condition is not enforced anywhere in the code.

We recommend that the finalizeInitialPotRaise() function should make sure that these team tokens are indeed staked and locked before finalizing the Pot. This will ensure added relief for the dear early investors and give more credibility to the protocol.