Lines of code

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

Vulnerability details

Impact

As it stands now, an eventual winner claims the winnings by means of claimWinningTickets function, passing an array of ticketIds. Total winning value is accumulated in claimedAmount variable inside a loop by calling repeatedly claimWinningTicket for each individual ticket.

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

Here is the claimWinningTicket function:

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

As we can see above, if the respective ticket happens to be non claimable, i.e. claimedAmount == 0, the function reverts, and subsequently, the entire transaction fails.

Accordingly, the resulting impact is:

Gas griefing (gas waste for the previously executed claims prior to encountering the non-claimable one). In case of a large number of winning tickets accumulated over a period of several months, the impact can be quite significant;
Poor UX / end user confusion.

Proof of Concept

The described outcome can occur as a result of the following 2 scenarios:

1. Trading a ticket on the secondary market.

According to the docs, tickets “Can be traded on the secondary market before or after the draw” (ref: https://docs.wenwin.com/wenwin-lottery/the-game/tickets).

Let’s consider the following chain of events. Player A has 10 winning tickets and wants to sell one of them on the secondary market. Player B negotiates with player A to buy a ticket with a high ticketId assuming that Player A, when calling claimWinningTickets, will populate the ticketIds array in ascending order. Then, Player B claims instantly the rewards for the newly purchased ticket. Accordingly, when Player A claims the winnings of all initially purchased ticket ids, including by mistake the id of the sold ticket to Player B (after all, he or she doesn’t expect any setback here), the transaction will revert and gas will be spent in vain. This behavior of Player A, of cause, is not guaranteed, but the risk is still there, and the protocol doesn’t provide any protection in this regard.
Event without having any malicious intention, Player B can purchase from Player A a ticket on the secondary market and innocently claim the winning. The impact could be the same.

2. Simply claiming all the purchased tickets, regardless whether they are winning or not (again, not guaranteed, but very likely).

Tools Used

Manual checking

Recommended Mitigation Steps

In principle, a well-crafted frontend app could mitigate the discussed issue, however, since the Frontend Operators will operate as third parties, it won’t be quite practical to rely on their implementation of the necessary protective measures. Therefore, it’s recommended to perform the required code refactoring in the smart contract itself.

Obviously, it’s up to the development team how exactly to go about this. A possible solution, though, would be to call claimable inside claimWinningTickets rather then in claimWinningTicket, and accordingly build up an array of claimable tickets and another one with non-claimable ones. Then, revert only in case not a single claimable ticket has been found. As long as there is at least one claimable ticket, it should be claimed as intended, subsequently updating the storage and emitting the respective events, for the claimable and non-claimable bunches of tickets respectively.

code-423n4 / 2023-03-wenwin-findings