Wenwin contest - adriro's results

Lines of code

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

Vulnerability details

The function calculateNewProfit present in the LotteryMath library is used when finalizing the current draw in the Lottery to track and update the currentNetProfit variable in the contract.

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

function calculateNewProfit(
    int256 oldProfit,
    uint256 ticketsSold,
    uint256 ticketPrice,
    bool jackpotWon,
    uint256 fixedJackpotSize,
    uint256 expectedPayout
)
    internal
    pure
    returns (int256 newProfit)
{
    uint256 ticketsSalesToPot = (ticketsSold * ticketPrice).getPercentage(TICKET_PRICE_TO_POT);
    newProfit = oldProfit + int256(ticketsSalesToPot);

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

    newProfit -= int256(expectedRewardsOut);
}

When jackpotWon == false, the function calls calculateMultiplier, which returns a scaled percentage, and misses to call getPercentage to scale down the value.

Impact

In the case that the jackpot isn't won in the current draw, the profit calculation will be wrongly scaled by a factor of PERCENTAGE_BASE (100_000) and will result in higher values that are offsetted by several orders of magnitude.

This error will cascade into rewards, as the currentNetProfit variable is used in the drawRewardSize function, which is used to calculate reward values and bonuses:

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

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

PoC

In the following test, a user purchases just 10 tickets that aren't the winning ticket (meaning no jackpot is won). The updated net profit ends up being -379965 DAI!

contract AuditTest is LotteryTestBase {
  function test_LotteryMath_calculateNewProfit_WrongCalculationWhenJackpotNotWon() public {
        // At start current profit is 0
        int256 currentProfit = lottery.currentNetProfit();
        assertEq(currentProfit, 0);

        // User buys some tickets
        address user = makeAddr("user");
        vm.startPrank(user);
        buySameTickets(lottery.currentDraw(), uint120(0x0F), address(0), 10);
        vm.stopPrank();

        // Finalize draw with no jackpot
        finalizeDraw(0x01020304);

        // Net profit is now -379965 * (10 ** 18)!
        currentProfit = lottery.currentNetProfit();
        assertEq(currentProfit, -379_965 ether);
    }
}

Recommendation

The calculation should use getPercentage to correctly scale down the percentage value:

function calculateNewProfit(
    int256 oldProfit,
    uint256 ticketsSold,
    uint256 ticketPrice,
    bool jackpotWon,
    uint256 fixedJackpotSize,
    uint256 expectedPayout
)
    internal
    pure
    returns (int256 newProfit)
{
    uint256 ticketsSalesToPot = (ticketsSold * ticketPrice).getPercentage(TICKET_PRICE_TO_POT);
    newProfit = oldProfit + int256(ticketsSalesToPot);

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

    newProfit -= int256(expectedRewardsOut);
}

Lines of code

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

Vulnerability details

A bad actor can trick a user into buying an unclaimed ticket and frontrun the selling or transfer of the NFT to claim the rewards associated with the ticket before the original transaction.

Impact

Given the described scenario, a bad actor can frontrun the transaction associated with the exchange of an unclaimed ticket with a transaction to claim the rewards associated with the ticket in the lottery. The malicious user will end up with both the ticket winnings and the sale revenue.

Proof of concept

Here "bad actor" is selling the unclaimed ticket NFT and "buyer" wishes to purchase it.

1. Bad actor lists Ticket NFT in secondary market .
2. Buyer sends transaction to purchase/exchange NFT.
3. Bad actor frontruns previous transaction to claim winnings from ticket.
4. Transaction from (2) succeeds and the purchase is executed but with an already claimed ticket.

Recommendation

• When a ticket is claimed, burn it or make it untransferable, or
• When a ticket is claimed, add a cooldown period and prevent any transfer during this period.

Quick example:

abstract contract Ticket is ITicket, ERC721 {
    uint256 constant TRANSFER_COOLDOWN = 1 hours;

    uint256 public override nextTicketId;
    mapping(uint256 => ITicket.TicketInfo) public override ticketsInfo;
    mapping(uint256 => uint256) private claimedAt;

    // solhint-disable-next-line no-empty-blocks
    constructor() ERC721("Wenwin Lottery Ticket", "WLT") { }

    function markAsClaimed(uint256 ticketId) internal {
        ticketsInfo[ticketId].claimed = true;
        claimedAt[ticketId] = block.timestamp;
    }

    function mint(address to, uint128 drawId, uint120 combination) internal returns (uint256 ticketId) {
        ticketId = nextTicketId++;
        ticketsInfo[ticketId] = TicketInfo(drawId, combination, false);
        _mint(to, ticketId);
    }

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 tokenId,
        uint256 batchSize
    ) internal override {
        super._beforeTokenTransfer(from, to, tokenId, batchSize);
        require(block.timestamp >= claimedAt[tokenId] + TRANSFER_COOLDOWN);
    }
}

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/LotterySetup.sol#L164-L176

Vulnerability details

When the lottery is initialized, fixed rewards are tightly packed in a 256 bit word. This is implemented in the packFixedRewards function:

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/LotterySetup.sol#L164-L176

function packFixedRewards(uint256[] memory rewards) private view returns (uint256 packed) {
    if (rewards.length != (selectionSize) || rewards[0] != 0) {
        revert InvalidFixedRewardSetup();
    }
    uint256 divisor = 10 ** (IERC20Metadata(address(rewardToken)).decimals() - 1);
    for (uint8 winTier = 1; winTier < selectionSize; ++winTier) {
        uint16 reward = uint16(rewards[winTier] / divisor);
        if ((rewards[winTier] % divisor) != 0) {
            revert InvalidFixedRewardSetup();
        }
        packed |= uint256(reward) << (winTier * 16);
    }
}

As shown in the previous snippet, the code will first divide the rewards by 10 ** (decimals - 1), meaning it will keep only the first decimal and discard any other precision, and then cast that result as an uint16. This casting will potentially overflow if the result is bigger than the max capacity of uint16, which doesn't represent a big value considering the protocol setup.

Impact

Fixed rewards greater than 6553.5 ether will silently overflow and wrap around. If the reward token is DAI or any other similar stable coin, then 6553.5 USD can't be considered a very high value and there is a real possibility of setting a value above that, which will end up in an overflow that won't raise any error.

Proof of Concept

The following test illustrates the issue. Fixed reward for tier 1 is just on the limit (6553.5 ether) and will be ok. Reward for tier 2 (6553.6 ether) will overflow and wrap around to zero.

contract AuditTest is LotteryTestBase {
    function test_LotterySetup_packFixedRewards_FixedRewardOverflow() public {
        uint256[] memory fixedRewards = new uint256[](SELECTION_SIZE);
        fixedRewards[0] = 0;
        fixedRewards[1] = 6553.5 ether;
        fixedRewards[2] = 6553.6 ether;
        firstDrawAt = block.timestamp + 3 * PERIOD;

        Lottery lottery = new Lottery(
            LotterySetupParams(
                rewardToken,
                LotteryDrawSchedule(firstDrawAt, PERIOD, COOL_DOWN_PERIOD),
                TICKET_PRICE,
                SELECTION_SIZE,
                SELECTION_MAX,
                EXPECTED_PAYOUT,
                fixedRewards
            ),
            playerRewardFirstDraw,
            playerRewardDecrease,
            rewardsToReferrersPerDraw,
            MAX_RN_FAILED_ATTEMPTS,
            MAX_RN_REQUEST_DELAY
        );

        // Fixed rewards for tier 1 are ok...
        uint256 winTier1Reward = lottery.currentRewardSize(1);
        assertEq(winTier1Reward, fixedRewards[1]);

        // Fixed rewards for tier 2 overflowed to zero
        uint256 winTier2Reward = lottery.currentRewardSize(2);
        assertEq(winTier2Reward, 0);
    }
}

Recommendation

If the protocol decides to pack rewards in smaller types, then ensure any overflow causes an explicit error by using, for example, the OpenZeppelin library SafeCast:

function packFixedRewards(uint256[] memory rewards) private view returns (uint256 packed) {
    if (rewards.length != (selectionSize) || rewards[0] != 0) {
        revert InvalidFixedRewardSetup();
    }
    uint256 divisor = 10 ** (IERC20Metadata(address(rewardToken)).decimals() - 1);
    for (uint8 winTier = 1; winTier < selectionSize; ++winTier) {
        uint16 reward = SafeCast.toUint16(rewards[winTier] / divisor);
        if ((rewards[winTier] % divisor) != 0) {
            revert InvalidFixedRewardSetup();
        }
        packed |= uint256(reward) << (winTier * 16);
    }
}

QA Report

• Formal Verification
• Non Critical Issues
• Low Issues

<a name="FV"></a>Formal Verification

Validity of reconstructed tickets

The following Certora rule proves that reconstructed tickets from a random source are valid within the accepted range of selectionSize and selectionMax parameters.

Results: https://prover.certora.com/output/78195/0f1385d89b704d8cbe588829e162028c?anonymousKey=3f8b6d024580c29b4a4eb11e6efc776cb8e45615

methods {
  isValidTicket(uint256,uint8,uint8) returns (bool) envfree
  reconstructTicket(uint256,uint8,uint8) returns (uint120) envfree
}

rule reconstructedTicketIsValid() {
  uint256 random;
  uint8 selectionSize;
  uint8 selectionMax;

  require selectionMax <= 120;
  require selectionSize <= 16 || selectionSize <= (selectionMax -1);

  uint256 ticket = reconstructTicket(random, selectionSize, selectionMax);

  assert isValidTicket(ticket, selectionSize, selectionMax), "reconstructed ticket is not valid";
}

<a name="NC"></a>Non Critical Issues

<a name="NC-1"></a>[NC-1] Import declarations should import specific symbols

Prefer import declarations that specify the symbol(s) using the form import {SYMBOL} from "SomeContract.sol" rather than importing the whole file

Instances (53):

File: src/Lottery.sol

5: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

6: import "@openzeppelin/contracts/utils/math/Math.sol";

7: import "src/ReferralSystem.sol";

8: import "src/RNSourceController.sol";

9: import "src/staking/Staking.sol";

10: import "src/LotterySetup.sol";

11: import "src/TicketUtils.sol";

File: src/LotteryMath.sol

5: import "src/interfaces/ILottery.sol";

6: import "src/PercentageMath.sol";

File: src/LotterySetup.sol

5: import "@openzeppelin/contracts/utils/math/Math.sol";

6: import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";

7: import "src/PercentageMath.sol";

8: import "src/LotteryToken.sol";

9: import "src/interfaces/ILotterySetup.sol";

10: import "src/Ticket.sol";

File: src/LotteryToken.sol

5: import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

6: import "src/interfaces/ILotteryToken.sol";

7: import "src/LotteryMath.sol";

File: src/RNSourceBase.sol

5: import "src/interfaces/IRNSource.sol";

File: src/RNSourceController.sol

5: import "@openzeppelin/contracts/access/Ownable2Step.sol";

6: import "src/interfaces/IRNSource.sol";

7: import "src/interfaces/IRNSourceController.sol";

File: src/ReferralSystem.sol

5: import "@openzeppelin/contracts/utils/math/Math.sol";

6: import "src/interfaces/IReferralSystem.sol";

7: import "src/PercentageMath.sol";

File: src/Ticket.sol

5: import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

6: import "src/interfaces/ITicket.sol";

File: src/VRFv2RNSource.sol

5: import "@chainlink/contracts/src/v0.8/VRFV2WrapperConsumerBase.sol";

6: import "src/interfaces/IVRFv2RNSource.sol";

7: import "src/RNSourceBase.sol";

File: src/interfaces/ILottery.sol

5: import "src/interfaces/ILotterySetup.sol";

6: import "src/interfaces/IRNSourceController.sol";

7: import "src/interfaces/ITicket.sol";

8: import "src/interfaces/IReferralSystem.sol";

File: src/interfaces/ILotterySetup.sol

5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

6: import "src/interfaces/ITicket.sol";

File: src/interfaces/ILotteryToken.sol

5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

File: src/interfaces/IRNSourceController.sol

5: import "@openzeppelin/contracts/access/Ownable2Step.sol";

6: import "src/interfaces/IRNSource.sol";

File: src/interfaces/IReferralSystem.sol

5: import "src/interfaces/ILotteryToken.sol";

File: src/interfaces/ITicket.sol

5: import "@openzeppelin/contracts/token/ERC721/IERC721.sol";

File: src/interfaces/IVRFv2RNSource.sol

5: import "src/interfaces/IRNSource.sol";

File: src/staking/StakedTokenLock.sol

5: import "@openzeppelin/contracts/access/Ownable2Step.sol";

6: import "src/staking/interfaces/IStakedTokenLock.sol";

7: import "src/staking/interfaces/IStaking.sol";

File: src/staking/Staking.sol

5: import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

6: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

7: import "src/interfaces/ILottery.sol";

8: import "src/LotteryMath.sol";

9: import "src/staking/interfaces/IStaking.sol";

File: src/staking/interfaces/IStakedTokenLock.sol

5: import "src/staking/interfaces/IStaking.sol";

File: src/staking/interfaces/IStaking.sol

5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

6: import "src/interfaces/ILottery.sol";

<a name="NC-2"></a>[NC-2] Use named parameters for mapping type declarations

Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18

Instances (17):

File: src/Lottery.sol

26:     mapping(address => uint256) private frontendDueTicketSales;

27:     mapping(uint128 => mapping(uint120 => uint256)) private unclaimedCount;

27:     mapping(uint128 => mapping(uint120 => uint256)) private unclaimedCount;

36:     mapping(uint128 => uint120) public override winningTicket;

37:     mapping(uint128 => mapping(uint8 => uint256)) public override winAmount;

37:     mapping(uint128 => mapping(uint8 => uint256)) public override winAmount;

39:     mapping(uint128 => uint256) public override ticketsSold;

File: src/RNSourceBase.sol

9:     mapping(uint256 => RandomnessRequest) internal requests;

File: src/ReferralSystem.sol

17:     mapping(uint128 => mapping(address => UnclaimedTicketsData)) public override unclaimedTickets;

17:     mapping(uint128 => mapping(address => UnclaimedTicketsData)) public override unclaimedTickets;

19:     mapping(uint128 => uint256) public override totalTicketsForReferrersPerDraw;

21:     mapping(uint128 => uint256) public override referrerRewardPerDrawForOneTicket;

23:     mapping(uint128 => uint256) public override playerRewardsPerDrawForOneTicket;

25:     mapping(uint128 => uint256) public override minimumEligibleReferrals;

File: src/Ticket.sol

14:     mapping(uint256 => ITicket.TicketInfo) public override ticketsInfo;

File: src/staking/Staking.sol

19:     mapping(address => uint256) public override userRewardPerTokenPaid;

20:     mapping(address => uint256) public override rewards;

<a name="NC-3"></a>[NC-3] Refactor common code across functions

• claimRewards and unclaimedRewards functions in the Lottery contract have a lot of duplicate functionality. Consider refactoring common code in a private function.

<a name="NC-4"></a>[NC-4] Unused local variables

Unused variables should be removed.

File: src/Lottery.sol

260:    uint256 winTier;

<a name="NC-5"></a>[NC-5] Use a modifier for access control

Consider using a modifier to implement access control instead of inlining the condition/requirement in the function's body.

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/RNSourceController.sol#L46-L49

function onRandomNumberFulfilled(uint256 randomNumber) external override {
    if (msg.sender != address(source)) {
        revert RandomNumberFulfillmentUnauthorized();
    }

<a name="NC-6"></a>[NC-6] Missing event for important parameter change

Important parameter or configuration changes should trigger an event to allow being tracked off-chain.

Instances (2):

• https://github.com/code-423n4/2023-03-wenwin/blob/main/src/staking/StakedTokenLock.sol#L24

• https://github.com/code-423n4/2023-03-wenwin/blob/main/src/staking/StakedTokenLock.sol#L37

<a name="L"></a>Low Issues

<a name="L-1"></a>[L-1] Contract files should define a locked compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Instances (2):

File: src/VRFv2RNSource.sol

3: pragma solidity ^0.8.7;

File: src/staking/StakedTokenLock.sol

3: pragma solidity ^0.8.17;

<a name="L-2"></a>[L-2] claimable function doesn't validate ticket draw is finished

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L159-L168

The function should validate that the draw associated with the ticket is already finished, as the function uses the winning ticket to run the calculation and this value will be undefined until the draw is finished and a ticket is selected.

<a name="L-3"></a>[L-3] DRAWS_PER_YEAR assumes one draw per week

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

The DRAWS_PER_YEAR constant is fixed at 52 assuming one draw lasts one week, while the lottery can be configured with an arbitrary draw period.

<a name="L-4"></a>[L-4] Simplify unpacking expression in fixedReward

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/LotterySetup.sol#L126-L127

The following calculation:

uint256 mask = uint256(type(uint16).max) << (winTier * 16);
uint256 extracted = (nonJackpotFixedRewards & mask) >> (winTier * 16);

Can be simplified using a single shift as:

uint256 extracted = (nonJackpotFixedRewards >> (winTier * 16)) & type(uint16).max;

<a name="L-5"></a>[L-5] First win tier is always empty in packFixedRewards

The rewards for the first win tier (tier 0) are always 0, meaning the lowest 16 bits of the packed rewards word are always 0 wasting this space. Consider offsetting the tier by 1 (store tier 1 in lowest 16 bits, tier 2 in next 16 bits, and so on) to take advantage of this space. This change also allows the protocol to support a max selection size of 17 instead of 16.

<a name="L-6"></a>[L-6] Limits in getMinimumEligibleReferralsFactorCalculation should be inclusive

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/ReferralSystem.sol#L111-L130

function getMinimumEligibleReferralsFactorCalculation(uint256 totalTicketsSoldPrevDraw)
    internal
    view
    virtual
    returns (uint256 minimumEligible)
{
    if (totalTicketsSoldPrevDraw < 10_000) {
        // 1%
        return totalTicketsSoldPrevDraw.getPercentage(PercentageMath.ONE_PERCENT);
    }
    if (totalTicketsSoldPrevDraw < 100_000) {
        // 0.75%
        return totalTicketsSoldPrevDraw.getPercentage(PercentageMath.ONE_PERCENT * 75 / 100);
    }
    if (totalTicketsSoldPrevDraw < 1_000_000) {
        // 0.5%
        return totalTicketsSoldPrevDraw.getPercentage(PercentageMath.ONE_PERCENT * 50 / 100);
    }
    return 5000;
}

The protocol documentation specifies that the total ticket sold limits should be inclusive during the calculation of the minimum referral eligibility. However, the different conditions in the if statements use a strict inequality to define the bounds to calculate the eligibility factor.

Change claimable function visibility

The claimable function in the Lottery contract is declared as external, but it is used within the contract as an external call. Consider changing the visibility to public in order to save gas by executing a local jump within the contract instead of an external call.

Storage variable is read twice in claimWinningTicket

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

unclaimedCount[ticketsInfo[ticketId].drawId][ticketsInfo[ticketId].combination]--;

Consider storing ticketsInfo[ticketId] as a local variable to prevent a second sload.

Unneeded storage variable reloading in claimPerDraw function

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/ReferralSystem.sol#L139 https://github.com/code-423n4/2023-03-wenwin/blob/main/src/ReferralSystem.sol#L145

The claimPerDraw function reads the unclaimedTickets storage variable to calculate and claim pending rewards. The function first reads it in line 139 to calculate the referrer side and then needlessly reloads it in line 145 to calculate the player side.

Unneeded call to _transferOwnership in StakedTokenLock constructor

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/staking/StakedTokenLock.sol#L17

The base contract Ownable2Step constructor will already give ownership to the caller. The call on line 17 can be safely removed to save gas.

Deposited balance in StakedTokenLock contract can be simplified with token balanceOf

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/staking/StakedTokenLock.sol#L14

The depositedBalance storage variable in the StakedTokenLock contract is superfluous since this value can be directly obtained from the staking token balanceOf function. Consider removing this variable and its usages to save gas.