kuprum - A raffle may be guaranteed won, or canceled despite reaching maxTicketSupply, thus depriving users from winning it

[sherlock-admin3]

kuprum

Medium

A raffle may be guaranteed won, or canceled despite reaching maxTicketSupply, thus depriving users from winning it

Summary

Insufficient validation in createRaffle allows to set different nonsensical combinations of raffle parameters. E.g. setting maxTickets == 1 allows the guaranteed win with a single purchased ticket; or setting minTickets >= maxTickets allows to cancel the raffle even when the number of bought tickets reaches maxTickets, thus depriving the users who bought tickets from winning the raffle.

Root Cause

Function createRaffle doesn't perform any validation wrt. the supplied parameters minTickets, maxTickets, maxHoldings besides checking that maxTickets != 0 and maxHoldings != 0. Different nonsensical combinations of these parameters may be set, in particular:

• setting maxTickets == 1 and bundling createRaffle and buyTickets for a single permitted ticket into the same transaction, will allow an admin to have a guaranteed win of the raffle. If the prize is large, there may be enough motivation to do that.
• setting minTickets >= maxTickets: as in the current Winnable UI the minTickets doesn't seem to be displayed, the users may participate not knowing of the nonsensical parameters. As we outline below, this combination allows to cancel a raffle even when maxTickets have been bought.

Wrt. to the latter, there is a secondary root cause, in functions _checkShouldDraw and _checkShouldCancel:

• Function _checkShouldDraw reverts if currentTicketSold < raffle.minTicketsThreshold. Thus even if maxTicketSupply is reached, but minTicketsThreshold >= maxTicketSupply a winner can't be ever drawn
• The revert conditions in functions _checkShouldDraw (currentTicketSold < raffle.minTicketsThreshold) and _checkShouldCancel (supply > raffle.minTicketsThreshold) are non-exclusive: if maxTicketSupply == minTicketsThreshold then both functions don't revert, thus offering a choice between either drawing or canceling the raffle, even when maxTicketSupply has been reached.

The case of minTicketsThreshold == maxTicketSupply seems actually reasonable, and thus can be set by an non-malicious admin. Taking into account that cancelRaffle is a non-admin-restricted function, and can be called by anyone, this setting can be exploited to cancel the raffle if the odds of winning don't seem favorable for to the interested party.

Internal pre-conditions

1. An admin needs to set certain combinations of parameters via createRaffle, e.g. to set minTicketsThreshold == maxTicketSupply

External pre-conditions

none

Attack Path

The first attack path (a malicious admin):

1. An admin calls createRaffle with minTickets == 0 and maxTickets == 1
2. In the same transaction the admin calls buyTickets and buys a single permissible ticket
3. In the same transaction the admin calls drawWinner (which is possible, because maxTicketSupply is reached.
4. The admin has a guaranteed win.

The second attack path (a non-malicious admin):

1. An admin when calling createRaffle sets minTickets == maxTickets (which seems reasonable), thus resulting for the raffle to have minTicketsThreshold == maxTicketSupply
2. The raffle proceeds as usual; maxTicketSupply is reached
3. Any participant, if they are unsatisfied with their odds of winning, call cancelRaffle, thus canceling it instead of the winner being drawn.

Impact

Depending on the attack path, it's either a guaranteed win (for a malicious admin), or a cancellation of the raffle that has reached the stage when it should be drawn, if the odds don't satisfy any interested party (with a non-malicious admin).

PoC

not required according to the rules.

Mitigation

We propose to perform more validation of the input parameters of createRaffle, e.g.

• require maxTickets to be greater than some reasonable minimum (e.g. 1000)
• require minTickets < maxTickets
• require maxHoldings < maxTickets (thus preventing accumulation of the odds of winning by a single address)

Different other restrictions may be imposed, depending on the protocol intentions.

Additionally, we recommend to make the revert conditions in functions _checkShouldDraw and _checkShouldCancel mutually exclusive: currentTicketSold <= raffle.minTicketsThreshold and supply > raffle.minTicketsThreshold respectively.

Duplicate of #26

[WangSecurity]

This will be duplicated with #26 and validated with Medium severity.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
Winnables/public-contracts#12