Wenwin contest - Cyfrin's results

Lines of code

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

Vulnerability details

Impact

LotteryMath.calculateNewProfit returns the wrong profit when there is no jackpot winner, and the library function is used when we update currentNetProfit of Lottery contract.

        currentNetProfit = LotteryMath.calculateNewProfit(
            currentNetProfit,
            ticketsSold[drawFinalized],
            ticketPrice,
            jackpotWinners > 0,
            fixedReward(selectionSize),
            expectedPayout
        );

Lottery.currentNetProfit is used during reward calculation, so it can ruin the main functionality of this protocol.

    function drawRewardSize(uint128 drawId, uint8 winTier) private view returns (uint256 rewardSize) {
        return LotteryMath.calculateReward(
            currentNetProfit,
            fixedReward(winTier), 
            fixedReward(selectionSize),
            ticketsSold[drawId],
            winTier == selectionSize,
            expectedPayout
        );
    }

Proof of Concept

In LotteryMath.calculateNewProfit, expectedRewardsOut is calculated as follows:

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

The calculation is not correct when there is no jackpot winner. When jackpotWon is false, ticketsSold * expectedPayout is the total payout in reward token, and then we need to apply a multiplier to the total payout, and the multiplier is calculateMultiplier(calculateExcessPot(oldProfit, fixedJackpotSize), ticketsSold, expectedPayout).

The calculation result is expectedRewardsOut, and it is also in reward token, so we should use PercentageMath instead of multiplying directly.

For coded PoC, I added this function in LotteryMath.sol and imported forge-std/console.sol for console log.

    function testCalculateNewProfit() public {
        int256 oldProfit = 0;
        uint256 ticketsSold = 1;
        uint256 ticketPrice = 5 ether;
        uint256 fixedJackpotSize = 1_000_000e18; // don't affect the profit when oldProfit is 0, use arbitrary value
        uint256 expectedPayout = 38e16;
        int256 newProfit = LotteryMath.calculateNewProfit(oldProfit, ticketsSold, ticketPrice, false, fixedJackpotSize, expectedPayout );

        uint256 TICKET_PRICE_TO_POT = 70_000;
        uint256 ticketsSalesToPot = PercentageMath.getPercentage(ticketsSold * ticketPrice, TICKET_PRICE_TO_POT);
        int256 expectedProfit = oldProfit + int256(ticketsSalesToPot);
        uint256 expectedRewardsOut = ticketsSold * expectedPayout; // full percent because oldProfit is 0
        expectedProfit -= int256(expectedRewardsOut);
        
        console.log("Calculated value (Decimal 15):");
        console.logInt(newProfit / 1e15); // use decimal 15 for output purpose

        console.log("Expected value (Decimal 15):");
        console.logInt(expectedProfit / 1e15);
    }

The result is as follows:

  Calculated value (Decimal 15):
  -37996500
  Expected value (Decimal 15):
  3120

Tools Used

Foundry

Recommended Mitigation Steps

Use PercentageMath instead of multiplying directly.

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

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/940066dc3c500cf745afc9e9381e131da7f98e88/src/staking/Staking.sol#L121 https://github.com/code-423n4/2023-03-wenwin/blob/940066dc3c500cf745afc9e9381e131da7f98e88/src/staking/Staking.sol#L51

Vulnerability details

Impact

Some rewards might be locked inside the staking contract forever if some tickets are bought while totalSupply == 0.

Proof of Concept

The Staking contract claims 20% of the total pot from the Lottery as a staking reward.

But it doesn’t track the rewards properly and some rewards might be locked like what we see below.

1. We start with totalSupply = 0 in Staking.sol
2. Before users stake, a user buys some tickets so lottery.nextTicketId() > 0.
3. After that, when users stake some amount, rewardPerTokenStored won’t be updated according to the bought tickets as totalSupply = 0.
4. But in _updateReward(), it sets as lastUpdateTicketId = lottery.nextTicketId() so the rewards for already bought tickets won't be added to the staking reward at all.
5. That amount of rewards will be transferred from Lottery to Staking.sol once a user calls getReward, but won't be transfered to the staker since the protocol isn't keeping track of that extra money.

To run the proof-of-code below, do the following:

1. Copy paste the below code to a file called poc.txt in the home directory of the codebase.
2. Run the following:

git apply poc.txt
forge test -m testWhenTotalSupplyOfStakingZeroRewardsLocked -vv

poc.txt:

diff --git a/test/Lottery.t.sol b/test/Lottery.t.sol
index 270e785..5166fd2 100644
--- a/test/Lottery.t.sol
+++ b/test/Lottery.t.sol
@@ -8,6 +8,7 @@ import "test/TestHelpers.sol";
 
 contract LotteryTest is LotteryTestBase {
     address public constant USER = address(123);
+    address public constant STAKER = address(69);
 
     function testFinalizeInitialPot(uint256 timestamp, uint256 initialSize) public {
         vm.warp(0);
@@ -85,6 +86,81 @@ contract LotteryTest is LotteryTestBase {
         lot.buyTickets(drawIds, tickets, FRONTEND_ADDRESS, address(0));
     }
 
+    function testWhenTotalSupplyOfStakingZeroRewardsLocked() public {
+        console.log("Lottery starting DAI balance", rewardToken.balanceOf(address(lottery)));
+        uint128 drawId = lottery.currentDraw();
+        uint256 numberOfTicketBatchesToBuy = 10; // initTickets buys 11 tickets
+
+        uint256[] memory ticketIds = new uint256[](numberOfTicketBatchesToBuy);
+        for (uint256 i = 0; i < numberOfTicketBatchesToBuy; ++i) {
+            ticketIds[i] = initTickets(drawId, 0x9A); // 0 out of the 4 winning numbers (0010011010)
+        }
+
+        // At this point, we have sold 110 tickets to USER
+        // So the pool should have gain some reward from the first 100 ticket sales
+        // We can even finalize the draw with nobody winning
+        finalizeDraw(0x01020304); // winning number: 1100100001 (801) (0x321)
+
+        IStaking staking = IStaking(lottery.stakingRewardRecipient());
+        ILotteryToken stakingToken = ILotteryToken(address(lottery.nativeToken()));
+
+        assertEq(staking.totalSupply(), 0);
+        // the lottery made 550 DAI (tickets are 5 DAI)
+        assertEq(rewardToken.balanceOf(address(lottery)), 1_000_550e18);
+        // 550 * 20% == 110 DAI rewards from staking... but can we get those out?
+        assertEq(lottery.unclaimedRewards(LotteryRewardType.STAKING), 110e18);
+        // If we stake now, STAKER won't get any money... So let's try that
+
+        // We can mint some stake token now, this could be from the pre-sale or otherwise.
+        uint256 stakeTokensMintAmount = 10;
+        // Ok, let's try to get the rewards out
+        vm.prank(address(lottery));
+        stakingToken.mint(STAKER, stakeTokensMintAmount);
+        vm.startPrank(STAKER);
+        stakingToken.approve(address(staking), stakeTokensMintAmount);
+        staking.stake(stakeTokensMintAmount);
+        staking.getReward();
+        vm.stopPrank();
+
+        assertEq(rewardToken.balanceOf(address(lottery)), 1_000_550e18);
+        assertEq(lottery.unclaimedRewards(LotteryRewardType.STAKING), 110e18);
+        // Ok that didn't work... perhaps we will get the rewards in the next draw? We are still staked
+
+        ticketIds = new uint256[](numberOfTicketBatchesToBuy);
+        for (uint256 i = 0; i < numberOfTicketBatchesToBuy; ++i) {
+            ticketIds[i] = initTickets(drawId + 1, 0x9A); // 0 out of the 4 winning numbers (0010011010)
+        }
+        // At this point, we have sold 220 tickets to USER, 110 for draw1 and 110 for draw2
+        finalizeDraw(0x01020304); // winning number: 1100100001 (801) (0x321)
+
+        // Ok! We were staked when a draw completed, perhaps now we will be able to get that 110 rewards!
+        // Where are we?
+        assertEq(rewardToken.balanceOf(STAKER), 0);
+        assertEq(rewardToken.balanceOf(address(lottery)), 1_001_100e18); // 550 + 550 = 1100 DAI
+        assertEq(rewardToken.balanceOf(address(staking)), 0);
+        // There should be 220 DAI of rewards avail to be withdrawn (1100 * .2 = 220)
+        assertEq(lottery.unclaimedRewards(LotteryRewardType.STAKING), 220e18);
+
+        // Moment of truth... and what happened?
+        console.log("staker DAI balance", rewardToken.balanceOf(STAKER));
+        vm.prank(STAKER);
+        staking.getReward();
+        console.log("staker DAI balance", rewardToken.balanceOf(STAKER));
+
+        // Ok, we claimed all the reward from the lottery
+        assertEq(lottery.unclaimedRewards(LotteryRewardType.STAKING), 0);
+        // And the Lottery gave away the 220 DAI in rewards! But... to where?
+        assertEq(rewardToken.balanceOf(address(lottery)), 1_000_880e18);
+
+        // Our staker only made the 110
+        assertEq(rewardToken.balanceOf(STAKER), 110e18);
+        // And they claimed everything, so they aren't owed anything
+        assertEq(staking.earned(STAKER), 0);
+        // But omg... Our staking contract now has the 110 DAI from when we originally bought tickets and we can't get
+        // it out!
+        assertEq(rewardToken.balanceOf(address(staking)), 110e18);
+    }
+
     function testBuyInvalidTicket() public {
         uint128 currentDraw = lottery.currentDraw();
 

Tools Used

Foundry

Recommended Mitigation Steps

In _updateReward(), lastUpdateTicketId shouldn’t be updated to lottery.nextTicketId() if totalSupply == 0.

    function _updateReward(address account) internal {
        uint256 currentRewardPerToken = rewardPerToken();
        rewardPerTokenStored = currentRewardPerToken;
+       if(totalSupply() != 0) {
            lastUpdateTicketId = lottery.nextTicketId();
+       }
        rewards[account] = earned(account);
        userRewardPerTokenPaid[account] = currentRewardPerToken;
    }

Low Severity Findings

[L-01] No check on lotterySetupParams.fixedRewardshaving having all indexes in array up to selectionSize

• https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/LotterySetup.sol#L91

If an admin accidentally configures a poor selection size, they would also have to update the fixed rewards, otherwise the reward tiers will be unfair. For example, if you update SELECTION_SIZE to 7 and SELECTION_MAX to 35 in LotteryTestBase.sol and then add this test to Lottery.t.sol

function testPatrickFixedRewards() public {
        console.log(lottery.fixedReward(0)); // 0
        console.log(lottery.fixedReward(1)); // 5.000000000000000000
        console.log(lottery.fixedReward(2)); // 10.000000000000000000
        console.log(lottery.fixedReward(3)); // 15.000000000000000000
        console.log(lottery.fixedReward(4)); // 0
        console.log(lottery.fixedReward(5)); // 0
        console.log(lottery.fixedReward(6)); // 0
        console.log(lottery.fixedReward(7)); // 300,300.000000000000000000 jackpot
    }

Mitigation

Run a require/check on deployment to see if all fixedRewards are accounted for based on the selection size.

[L-02] An attacker can make lastRequestFulfilled = false by frontrunning executeDraw() before source is initialized

• https://github.com/code-423n4/2023-03-wenwin/blob/940066dc3c500cf745afc9e9381e131da7f98e88/src/Lottery.sol#L133

If an attacker front runs RNSourceController.requestRandomNumber before the source is initialized by admin, lastRequestFulfilled will be false in requestRandomNumberFromSource() and the contract would start working after waiting for 5 hours and calling retry().

1. The RNSourceController was created by the admin.
2. An attacker front runs executeDraw() before the source is initialized.
3. Then lastRequestFulfilled = false in requestRandomNumberFromSource() and it won't revert as source.requestRandomNumber() is within try-catch
4. As lastRequestFulfilled = false, requestRandomNumber() will revert and it will be the same after the source is initialized by the admin because initSource() doesn't reset lastRequestFulfilled.
5. As a result, the contract should call retry() to start working after maxRequestDelay

Mitigation

initSource() should set lastRequestFulfilled = true, maxFailedAttempts = 0, maxRequestDelay = 0 like swapSource()

Otherwise, we would consider setting the source inside the constructor.

Or, we could have executeDraw not be callable unless source is initialized.

[L-03] Some percent of frontend rewards wouldn't be claimed forever for some cases and it should be added to net profit

• https://github.com/code-423n4/2023-03-wenwin/blob/940066dc3c500cf745afc9e9381e131da7f98e88/src/Lottery.sol#L113

In buyTickets(), it tracks frontendDueTicketSales for frontend and every frontend can claim rewards(10%) by calling claimRewards().

But if frontend == address(0) in buyTickets(), the tickets will be added to address(0) and it won’t be claimed forever.

It means the rewards will be locked inside the contract as there is no logic to recover such rewards(like increasing net profit accordingly).

Mitigation

Add a check to make sure frontend != address(0)

Informational / Non-Crit Findings

[I-01] Prizes are said to be both dynamic & static

• https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L20
• https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/interfaces/ILotterySetup.sol#L76

In Lottery.sol it says:

/// All prizes are dynamic and dependant on the actual ticket sales.

Yet, in ILotterySetup.sol it clearly says:

// @dev Array of fixed rewards per each non jackpot win

It cannot be true that all prizes are dynamic yes rewards for non-jackpot wins are fixed.

Mitigation

Update the comment in Lottery.sol to reflect that some prizes are fixed.

[I-02] Inflation rate is not correct based on what's in ReferralSystemConfig.sol

In the wenwin docs it's said the inflation rate is a "hypothetical max" of 8% for the first year.

With current parameters, the first week can be a max of 8.6% as shown by the code below (if added to Lottery.t.sol):

        function testInflationHigherThanAnticipated() public {
        uint256 startingLotSupply = lotteryToken.totalSupply();
        console.log("Starting supply: ", startingLotSupply);

        uint128 currentDraw = lottery.currentDraw();

        vm.startPrank(USER);
        uint256 amount = 500_000_000 ether;
        rewardToken.mint(amount); // DAI
        rewardToken.approve(address(lottery), amount);
        uint128 targetDrawId = currentDraw + 100_000_000; // 100_000_000 weeks away, doesn't matter though.
        uint256 drawIdsSize = 1;
        uint128[] memory drawIds = new uint128[](drawIdsSize);
        uint120[] memory tickets = new uint120[](drawIdsSize);
        for (uint256 i = 0; i < drawIdsSize; i++) {
            drawIds[i] = targetDrawId;
            tickets[i] = uint120(0x0F);
        }
        // User is going to get all the inflation LOT
        address frontend = USER;
        address referer = USER;
        lottery.buyTickets(drawIds, tickets, frontend, referer);
        vm.stopPrank();

        vm.warp(block.timestamp + 60 * 60 * 24);
        lottery.executeDraw();

        uint256 randomNumber = 0x00000000;
        vm.prank(randomNumberSource);
        lottery.onRandomNumberFulfilled(randomNumber);
        uint128[] memory targetDrawIds = new uint128[](1);
        targetDrawIds[0] = currentDraw;

        vm.startPrank(USER);
        lottery.claimReferralReward(targetDrawIds);
        // 1,661,538.500000000000000000 LOT
        vm.stopPrank();

        uint256 endingSupply = lotteryToken.totalSupply();

        // 8% inflation across 52 weeks is 0.153846153846154% of initial supply a week
        // 8 / 52 = 0.153846153846154
        assertFalse(endingSupply <= ((startingLotSupply * 153_846_153_846_154) / 1_000_000_000_000_000));
        console.log("Starting supply: ", startingLotSupply);
        console.log("Ending supply: ", endingSupply);
        console.log("Inflation: ", endingSupply - startingLotSupply); // 8.6%
        console.log("Expected inflation: ", (startingLotSupply * 153_846_153_846_154) / 1_000_000_000_000_000);
    }

[I-03] Comment wrong for reconstructTicket

• https://github.com/code-423n4/2023-03-wenwin/blob/940066dc3c500cf745afc9e9381e131da7f98e88/src/TicketUtils.sol#L36

The comment says it will divide by 2^8=256 but the implementation divides by total count of 35.