code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Buyer on the secondary market cannot safely buy a winning ticket after a draw.

Proof of Concept

Once a winning ticket has been claimed it is marked as claimed to prevent double claiming

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

This is then checked in claimable() when attempting to claim a ticket.

function claimable(uint256 ticketId) external view override returns (uint256 claimableAmount, uint8 winTier) {
        TicketInfo memory ticketInfo = ticketsInfo[ticketId];
        if (!ticketInfo.claimed) {
            uint120 _winningTicket = winningTicket[ticketInfo.drawId];
            winTier = TicketUtils.ticketWinTier(ticketInfo.combination, _winningTicket, selectionSize, selectionMax);
            if (block.timestamp <= ticketRegistrationDeadline(ticketInfo.drawId + LotteryMath.DRAWS_PER_YEAR)) {
                claimableAmount = winAmount[ticketInfo.drawId][winTier];
            }
        }
    }

Setting ticketsInfo[ticketId].claimed to true allows the NFT to still be transferred between accounts.

Consider the following scenario

User A has 1 winning ticket NFT, which has a value of 100 DAI if claimed in the lottery
User A places a sell order on a secondary NFT marketplace to sell the ticket for 95 DAI
User B sees the sell order, verifies the ticket has not been claimed yet, and submits a transaction to buy the ticket
User A sees this transaction from user B and submits a transaction calling claimWinningTickets() with a higher gas price than user B.
If User A transaction is executed first, the winnings goes to user A, The buy order from user B still goes through, and they cannot claim the ticket, thus losing out on 95 DAI

The docs state that tickets are intended to be traded on the secondary market before or after the draw

With the current implementation, winning tickets cannot be safely traded on a secondary market as the buyer can lose funds to a malicious seller.

Tools Used

Manual review

Recommended Mitigation Steps

Disable the transfer of winning tickets after they have been claimed.

c4-judge commented 1 year ago

thereksfour marked the issue as duplicate of #425