Wenwin contest - horsefacts's results

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L170-L177

Vulnerability details

The Wenwin docs note that tickets "can be traded on the secondary market before or after the draw," since they are standard ERC721 tokens.

After a ticket draw, the owner of a winning ticket may call Lottery#claimWinningTickets, which transfers lottery winnings to the ticket owner:

Lottery#claimWinningTickets:

    function claimWinningTickets(uint256[] calldata ticketIds) external override returns (uint256 claimedAmount) {
        uint256 totalTickets = ticketIds.length;
        for (uint256 i = 0; i < totalTickets; ++i) {
            claimedAmount += claimWinningTicket(ticketIds[i]);
        }
        rewardToken.safeTransfer(msg.sender, claimedAmount);
    }

A malicious winner can thus list their winning ticket on the secondary market and frontrun purchase transactions to collect both their winning reward and the secondary sale price of their ticket.

Scenario:

1. Mallory buys a Wenwin ticket.
2. Following the draw, the ticket is revealed as a Match 6 winner, worth 1500 DAI.
3. Mallory lists the winning ticket on a secondary marketplace like OpenSea for 1000 DAI.
4. Alice sees the ticket listed at a discount, and submits a transaction to purchase Mallory's listed ticket.
5. Mallory frontruns Alice's transaction and claims ticket winnings before the token is transferred to Alice.
6. Mallory receives both lottery winnings and secondary sale price. Alice cannot claim the lottery winnings from the ticket she purchased.

Recommendation: Consider a two step process for claiming awards: the end user can first submit their intent to claim, wait a fixed period of time, then finalize their claim.

Additionally, consider using Ticket token ERC721 metadata to clearly visually distinguish claimed and unclaimed tickets to mitigate secondary market scams. (For example, change the background color of claimed vs unclaimed ticket tokens). The current implementation has no token metadata for Ticket ERC721s, which will make it difficult to distinguish claimed/unclaimed and pre-draw/post-draw tickets trading on secondary marketplaces. Even a simple dynamic token SVG could mitigate many secondary market scams.

Low

LotteryMath: Hardcoded DRAWS_PER_YEAR may be incorrect

The LotteryMath helper assumes a hardcoded value of 52 DRAWS_PER_YEAR, equivalent to one draw per week:

LotteryMath#DRAWS_PER_YEAR:

    /// @dev Number of lottery draws per year
    uint128 public constant DRAWS_PER_YEAR = 52;

However, the draw period and schedule parameters are configurable in the LotterySetup contract, and it's possible to create a lottery with more or less than 52 draws per year.

This affects unclaimed ticket deadlines: unclaimed jackpots may be returned to the pot after 52 draw periods, not necessarily 52 weeks:

Lottery#returnUnclaimedJackpotToThePot

    function returnUnclaimedJackpotToThePot() private {
        if (currentDraw >= LotteryMath.DRAWS_PER_YEAR) {
            uint128 drawId = currentDraw - LotteryMath.DRAWS_PER_YEAR;
            uint256 unclaimedJackpotTickets = unclaimedCount[drawId][winningTicket[drawId]];
            currentNetProfit += int256(unclaimedJackpotTickets * winAmount[drawId][selectionSize]);
        }
    }

If a lottery is configured with frequent draws, the timeline to return unclaimed payouts may be shorter than expected.

StakedTokenLock: Deposit deadline and lock duration are not validated

The StakedTokenLock contract is designed as a lock for Wenwin team tokens, to be "pre-staked and locked for 1 year" according to Wenwin docs.

However, the 1 year lockDuration and depositDeadline parameters are passed as constructor parameters and are not validated in the lock contract:

StakedTokenLock:

    constructor(address _stakedToken, uint256 _depositDeadline, uint256 _lockDuration) {
        _transferOwnership(msg.sender);
        stakedToken = IStaking(_stakedToken);
        rewardsToken = stakedToken.rewardsToken();
        depositDeadline = _depositDeadline;
        lockDuration = _lockDuration;
    }

Although these parameters would be publicly visible after deployment, the Wenwin team can deploy this contract with a zero lock duration. Consider validating that lock duration is at least 1 year.

Ticket: Missing ERC721 metadata

The Ticket ERC721 does not implement a meaningful tokenURI function returning token metadata. This is not required my the ERC721 standard, but since Wenwin tickets are expected to trade on secondary markets, it's a good idea to implement meaningful metadata for your use case to mitigate secondary market scams.

In particular, token metadata should show whether a ticket's draw is completed, whether the ticket is a winner, and whether or not the ticket's payout has already been claimed. Although it's possible to check all this data on chain, exposing it to secondary marketplaces through token metadata will help prevent scams on the secondary market.

NC

Lottery: Use new named mapping parameter syntax

Since these contracts are using Solidity >0.8.18, it's possible to use named mapping parameters. Consider using these to enhance readability of nested mappings:

Lottery.sol

    mapping(address => uint256) private frontendDueTicketSales;
    mapping(uint128 => mapping(uint120 => uint256)) private unclaimedCount;

Suggested:

    mapping(address frontend => uint256 totalSales) private frontendDueTicketSales;
    mapping(uint128 draw => mapping(uint120 ticket => uint256 unclaimed)) private unclaimedCount;

LotteryToken: Unused import

LotteryToken.sol imports but does not use LotteryMath.sol:

import "src/LotteryMath.sol";

TicketUtils: Comment typo

There's a small comment typo in TicketUtils:

    /// @return isValid Is ticked valid