Skip to content

Latest commit

 

History

History
107 lines (81 loc) · 5.17 KB

H-01.md

File metadata and controls

107 lines (81 loc) · 5.17 KB
Raw

Winner is unable to claim winnings at the very end of the claimable period

The protocol states that a winner has 1 year to claim prizes, after that prizes go back to the pot if not claimed. The protocol implements this time frame as 52 draws as they expect to configure one draw per week (DRAWS_PER_YEAR constant defined in LotteryMath).

From the docs:

Winning tickets have up to 1 year to claim their prize. If a prize is not claimed before this period, the unclaimed prize money will go back to the Prize Pot.

However, technically this is not correctly implemented, as the protocol will set the limit to 52 periods but will offset this limit with the cooldown period. The claimable function uses the ticketRegistrationDeadline function to calculate the limit:

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

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

However, the ticketRegistrationDeadline function offsets this limit the cooldown period:

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

function ticketRegistrationDeadline(uint128 drawId) public view override returns (uint256 time) {
    time = drawScheduledAt(drawId) - drawCoolDownPeriod;
}

This means that a user holding an unclaimed winner ticket won't be able to claim their prize at the very end of the claimable period.

----------------------------------------------------------------------------------------------
            |                                             |                                    |
Ticket draw |              ...  +50 draws ...             |     +51 draw      |  Cool down     |
            |                                             |                                    |
-----------------------------------------------------------------------------------------------
            ^                                             ^                   ^                ^
        ticket draw ends                            start of last       user can't claim   should end here
    claimable period starts                        claimable period       from here

Impact

Ticket winners won't be able to claim their prizes at the very end of the claimable period and will lose their funds while they still technically should have enough time until the last claimable draw finishes.

Proof of Concept

The following test reproduces the issue. A user wins the jackpot and is entitled to the jackpot prize. We then advance time until the last claimable period minus the cooldown period. The user now has nothing to claim, the call to claimWinningTickets will revert.

contract AuditTest is LotteryTestBase {
    function test_Lottery_claimRewards_RewardLostAtEndOfClaimablePeriod() public {
        address user = makeAddr("user");
        uint120 ticket = uint120(0x0321);
        uint256 randomNumber = 0x01020304;

        vm.startPrank(user);
        uint256[] memory ticketIds = buySameTickets(lottery.currentDraw(), ticket, address(0), 1);
        vm.stopPrank();

        finalizeDraw(randomNumber);

        uint256 claimableAmount;
        uint256 jackpotPrize = lottery.currentRewardSize(SELECTION_SIZE);

        // User won jackpot and can claim reward
        (claimableAmount,) = lottery.claimable(ticketIds[0]);
        assertEq(claimableAmount, jackpotPrize);

        // Advance 52 period minus the cooldown period
        vm.warp(block.timestamp + 52 * PERIOD - COOL_DOWN_PERIOD);

        // User claimable is now 0! Should still be able to claim until the last period finishes
        (claimableAmount,) = lottery.claimable(ticketIds[0]);
        assertEq(claimableAmount, 0);

        // Claiming will revert with NothingToClaim error
        vm.prank(user);
        vm.expectRevert(abi.encodeWithSelector(NothingToClaim.selector, ticketIds[0]));
        lottery.claimWinningTickets(ticketIds);
    }
}

Recommendation

The claimable function should use drawScheduledAt instead of ticketRegistrationDeadline to correctly implement the claimable limit:

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 <= drawScheduledAt(ticketInfo.drawId + LotteryMath.DRAWS_PER_YEAR)) {
            claimableAmount = winAmount[ticketInfo.drawId][winTier];
        }
    }
}