Wenwin contest - hl_'s results

Lines of code

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

Vulnerability details

Impact

Secondary NFT buyer can lose funds if prize was already claimed before ownership.

Proof of Concept

Function claimWinningTickets of Lottery.sol allows players to claim their prize if their ticket is the winning ticket. After the prize is claimed, the ticket will be marked as claimed to prevent double claiming.

File: 2023-03-wenwin/src/Lottery.sol (L170 - 176)

 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);
    }

File: 2023-03-wenwin/src/Lottery.sol (L259 - 269)

function claimWinningTicket(uint256 ticketId) private onlyTicketOwner(ticketId) returns (uint256 claimedAmount) {
        uint256 winTier;
        (claimedAmount, winTier) = this.claimable(ticketId);
        if (claimedAmount == 0) {
            revert NothingToClaim(ticketId);
        }

        unclaimedCount[ticketsInfo[ticketId].drawId][ticketsInfo[ticketId].combination]--;
        markAsClaimed(ticketId);
        emit ClaimedTicket(msg.sender, ticketId, claimedAmount);
    }

Given that the NFT is tradable in a secondary market, it is reasonable to assume that a buyer would want to buy the NFT if it has value (i.e. eligible for a prize).

Consider a case where:

1. A seller has a winning ticket and is entitled to the prize.
2. Seller places a sell order in secondary market for a price lower than the value of the prize.
3. Buyer sees a good deal and wants to buy the NFT.
4. When the buyer submits a buy order, the seller front-runs this buy transaction and submits the claimWinningTickets function.
5. As the seller's transaction is executed first, the prize goes to the seller. Thereafter, the buyer's transaction is executed and the ownership of the NFT transfers to the buyer. However, the buyer is no longer able to claim the prize as it was already claimed by the seller.
6. NFT Buyer loses funds in this regard.

Tools Used

Manual review.

Recommended Mitigation Steps

Disable NFT transfer/ burn the NFT once the prize is claimed.

Table of contents

• [N-01] Insufficient coverage
• [N-02] NatSpec comments should be increased in contracts
• [N-03] File does not contain an SPDX identifier
• [N-04] Solidity complier version too new
• [N-05] Exact functions not imported
• [L-01] safemint() should be used rather than mint()
• [L-02] Missing check in First draw schedule to ensure time not set in the past
• [L-03] Check for zero address or amount == 0 in LotterySetupParams constructor
• [L-04] Avoid writing division before multiplication
• [L-05] Unbounded Loop when buying an excessive amount of tickets

[N-01] Insufficient coverage

The test coverage rate of the project is 80.69%. Testing all functions is best practice in terms of security criteria.

[N-02] NatSpec comments should be increased in contracts

In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI). It is clearly stated in the Solidity official documentation.

[N-03] File does not contain an SPDX identifier

Missing first line of code

For example:

File:    2023-03-wenwin/src/Lottery.sol 
1:    // SPDX-License-Identifier: UNLICENSED

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

[N-04] Solidity complier version too new

The complier version used in the contract is 0.8.19 which is very new and hence not yet thoroughly tested for possible bugs, etc. It is best practice to use a more thoroughly tested complier version e.g. 0.8.16

For example:

File:    2023-03-wenwin/src/Lottery.sol 
3:    pragma solidity 0.8.19;

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

[N-05] Exact functions not imported

It is best practice for exact functions to be imported.

For example:

File:    2023-03-wenwin/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";

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

[L-01] safemint() should be used rather than mint()

mint() is discouraged in favor of safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function.

File:    2023-03-wenwin/src/Lottery.sol 
192:      ticketId = mint(msg.sender, drawId, ticket);

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

[L-02] Missing check in First draw schedule to ensure time not set in the past

Per the constructor of LotterySetup.sol, protocol owner sets the first draw schedule:

File:    2023-03-wenwin/src/LotterySetup.sol 
83:    firstDrawSchedule = lotterySetupParams.drawSchedule.firstDrawScheduledAt;

There should be a check for firstDrawSchedule > block.timestamp to ensure that firstDrawSchedule is not set in the past by mistake.

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

[L-03] Check for zero address or amount == 0 in LotterySetupParams constructor

It is best practice for every variable to be checked for 0 value in case of a typo error.

File:     2023-03-wenwin/src/LotterySetup.sol 
86:    ticketPrice = lotterySetupParams.ticketPrice;
87:    selectionSize = lotterySetupParams.selectionSize;
88:    selectionMax = lotterySetupParams.selectionMax;
89:    expectedPayout = lotterySetupParams.expectedPayout;

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

[L-04] Avoid writing division before multiplication

File:    2023-03-wenwin/src/ReferralSystem.sol
122:    // 0.75%
123:    return totalTicketsSoldPrevDraw.getPercentage(PercentageMath.ONE_PERCENT * 75 / 100);
124:    }
125:    if (totalTicketsSoldPrevDraw < 1_000_000) {
126:    // 0.5%
127:    return totalTicketsSoldPrevDraw.getPercentage(PercentageMath.ONE_PERCENT * 50 / 100);
128:    } 

Write as the following instead: totalTicketsSoldPrevDraw.getPercentage(750) totalTicketsSoldPrevDraw.getPercentage(500)

[L-05] Unbounded Loop when buying an excessive amount of tickets

The contract should be capable to take a large purchase from whales without reverting due to excessive gas.

The buyTickets() function in Lottery.sol loops through the drawId length, which refers to the number of tickets that a player wants to buy.

If drawIds.length is too high due to the same player buying a massive number of tickets at one go, the function will revert due to reaching maximum gas limit. Consider breaking up the loop into smaller loops so that each execution can be successful.

File:    2023-03-wenwin/src/Lottery.sol 
124:    ticketIds = new uint256[](tickets.length);
125:    for (uint256 i = 0; i < drawIds.length; ++i) {
            ticketIds[i] = registerTicket(drawIds[i], 126:    tickets[i], frontend, referrer);
127:    }

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