Buying tickets after the winning ticket was set

[code423n4]

Lines of code

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

Vulnerability details

Impact

A user can buy a ticket after the winning ticket was set by inserting the transaction in the same block, which allows them to win the jackpot. In addition, it allows for winning more than the intended jackpot amount.

Root-Cause

Buying tickets is blocked with the modifier beforeTicketRegistrationDeadline which reverts if block.timestamp is larger than ticketRegistrationDeadline(drawId) which is drawScheduledAt(drawId) - drawCoolDownPeriod

The draw itself reverts if block.timestamp < drawScheduledAt(currentDraw).

This means that if drawCoolDownPeriod is 0 (allowed by the constructor) and block.timestamp equals drawScheduledAt(currentDraw) a ticket can be purchased in this block, even after the drawing happens.

Result

This allows for someone to call buyTickets with currentDraw and the winning ticket. If this is done also after receiveRandomNumber is called (which in some random source implementation seems to be the case, as this is called from within executeDraw, or in the case the random number is also provided in the same block) than winAmount[drawFinalized][selectionSize] is set to be drawRewardSize(drawFinalized, selectionSize) / jackpotWinners; so that each jackpot winner receives their share from the pot. The problem is that the tickets were purchased after this calculation, causing it to be not correct and allowing for the withdrawal of more funds than in the pot.

Proof of Concept

• https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#LL209C54-L209C117
• https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/LotterySetup.sol#L114
• https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L135
• https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/VRFv2RNSource.sol#L32

Tools Used

Auditing

Recommended Mitigation Steps

Change beforeTicketRegistrationDeadline modifier to actually check it's before the registration deadline, i.e. make it like so:

modifier beforeTicketRegistrationDeadline(uint128 drawId) {
        // slither-disable-next-line timestamp
        if (block.timestamp >= ticketRegistrationDeadline(drawId)) {
            revert TicketRegistrationClosed(drawId);
        }
        _;
    }

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[c4-judge]

thereksfour marked the issue as primary issue

[rand0c0des]

This is a good finding. I agree with medium severity because of highly unlikely scenario of deploying lottery with cool down period of 0, plus the way current oracles are working.

[c4-sponsor]

rand0c0des marked the issue as sponsor confirmed

[c4-judge]

thereksfour marked issue #343 as primary and marked this issue as a duplicate of 343

[c4-judge]

thereksfour marked the issue as satisfactory