Platform: Code4rena
Start Date: 06/03/2023
Pot Size: $36,500 USDC
Total HM: 8
Participants: 93
Period: 3 days
Judge: cccz
Total Solo HM: 3
Id: 218
League: ETH
Rank: 2/93
Findings: 1
Award: $4,040.02
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: NoamYakov
4040.0191 USDC - $4,040.02
https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L135-L137 https://github.com/code-423n4/2023-03-wenwin/blob/main/src/Lottery.sol#L164-L166
According to the documentation:
"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."
As part of this mechanism, the Lottery.executeDraw() function (which internally calls Lottery.returnUnclaimedJackpotToThePot()) increases Lottery.currentNetProfit by the prize of any 1 year old unclaimed jackpot ticket. At this point, the contract thinks it still hold that prize and it's going to include it in the prize pot of the next draw. This function can only be called when block.timestamp >= drawScheduledAt(currentDraw).
On the other side, the Lottery.claimWinningTickets() function (which internally calls Lottery.claimWinningTicket() and Lottery.claimable()) sends the prize to the owner of a jackpot ticket only if it isn't 1 year old (if block.timestamp <= ticketRegistrationDeadline(ticketInfo.drawId + LotteryMath.DRAWS_PER_YEAR)).
However, if Lottery.drawCoolDownPeriod is zero, both of these condition can pass on the same time - when the block.timestamp is exactly the scheduled draw date of the draw that takes place exactly 1 year after the draw in which the jackpot ticket has won.
In this edge case, the owner of the jackpot ticket can call Lottery.executeDraw(), letting the Lottery think the prize wasn't claimed, followed by Lottery.claimWinningTickets(), claiming the prize, all in the same transaction.
Let's say Eve buys a ticket to draw #1 in a Lottery contract where Lottery.drawCoolDownPeriod equals zero, and win the jackpot. Now, Eve can wait 1 year and then, when block.timestamp equals the scheduled draw date of draw #53 (which is also the registration deadline of draw #53), run a transaction that will do the following:
lottery.executeDraw() lottery.claimWinningTickets([eveJackpotTicketId])
This will send Eve her prize, but will also leave the contract insolvent.
Manual code review.
Fix Lottery.claimable() to set claimableAmount to winAmount[ticketInfo.drawId][winTier] only if block.timestamp < ticketRegistrationDeadline(ticketInfo.drawId + LotteryMath.DRAWS_PER_YEAR) (strictly less than).
#0 - c4-judge
2023-03-10T10:31:51Z
thereksfour marked the issue as duplicate of #141
#1 - d3e4
2023-03-11T11:23:41Z
I don't think this is a duplicate of #141. #141 concerns front-running the draw with a purchase, whereas this issue concerns "back-running" the return of unclaimed jackpots with a claim. However, both issues would be resolved by requiring that drawCoolDownPeriod > 0.
#2 - thereksfour
2023-03-12T05:48:38Z
It is actually one of the exceptions caused by drawCoolDownPeriod == 0, and both can be resolved by setting drawCoolDownPeriod > 0, so consider them to be duplicates
#3 - noamyakov
2023-03-14T22:53:03Z
Hi @thereksfour. Like @d3e4 said, I don't think this submission should be considered a duplicate of #141.
These two submissions present two different attack vectors on two different mechanisms in the protocol:
It's true that both of these vulnerabilities can be fixed by requiring a non-zero drawCoolDownPeriod, but it doesn't necessarily mean they are the same issue. It only means that both of those vulnerable mechanisms don't correctly handle this specific edge case - each in its own way.
It’s important that you’ll consider this submission as "not duplicate" of #141 because it’s the only submission that presents this vulnerability (a solo finding).
In addition, I think that although #141 was confirmed as medium risk issue, this report should remain with high risk, because of the following reasons:
While #141 introduces the ability to "just" win a draw unfairly, this report introduces the ability to break the system's solvency - and make permanent damage to any future attempt to claim rewards.
#141 assumes that the oracle operates in a specific way. The sponsor @rand0c0des has even claimed that it cannot be exploited with the current oracle implementations (see #141). This report has no such assumptions.
Maybe drawCoolDownPeriod is planned to be non-zero on Wenwin's own lottery system, but this project was also made for developers to build their own lottery systems - and they can definitely use zero.
"Wenwin is a decentralized gaming protocol that provides developers with the ability to create chance-based games on the blockchain. The first product is Lottery, and it is the subject of this audit contest."
#4 - c4-judge
2023-03-15T03:06:37Z
thereksfour marked the issue as not a duplicate
#5 - c4-judge
2023-03-15T03:06:43Z
thereksfour changed the severity to 2 (Med Risk)
#6 - c4-judge
2023-03-15T03:06:50Z
thereksfour marked the issue as primary issue
#7 - thereksfour
2023-03-15T03:12:55Z
Agreed not a duplicate, but due to the requirement drawCoolDownPeriod == 0, consider Medium
#8 - rand0c0des
2023-03-15T14:47:14Z
I think this is medium, it requires coolDownPeriod of 0 + needs the block executed at the actual time scheduled which might not happen that often.
#9 - c4-sponsor
2023-03-15T14:47:22Z
rand0c0des marked the issue as disagree with severity
#10 - noamyakov
2023-03-15T16:22:38Z
Hey @rand0c0des. I would like to respond on your comment.
Miners can execute transactions on their blocks, so they have full control over block.timestamp. An attacker can mine a block and make the transaction execute exactly when block.timestamp triggers this vulnerability.
And as to drawCoolDownPeriod, the attack described in this report can work only if it is zero. But since this value is set by owners of lottery systems, we can't say that likelihood of a successful attack is low - what we can say is that the likelihood is 100% on lottery systems that have this value set to zero. It is very likely that some of the developers who create their own lottery systems using Wenwin's code base, will choose a drawCoolDownPeriod of zero - what makes this vulnerability very likely to be exploited.
After presenting all the facts, please let me know if you (@thereksfour @rand0c0des) still think that this report should be rewarded as a medium risk issue or as a high risk issue. I'll except any decision - but please make sure you consider all the facts.
#11 - thereksfour
2023-03-16T01:49:02Z
Miners can execute transactions on their blocks, so they have full control over block.timestamp. An attacker can mine a block and make the transaction execute exactly when block.timestamp triggers this vulnerability.
The likelihood would be considered low if miner support is required, but since it can happen under normal conditions, the likelihood is considered medium.
It is very likely that some of the developers who create their own lottery systems using Wenwin's code base, will choose a drawCoolDownPeriod of zero
This is still based on an assumption. I would say that as long as drawCoolDownPeriod is not hardcoded to 0, then the likelihood of drawCoolDownPeriod == 0 is considered medium
Conclusion: still considered med-risk
#12 - rand0c0des
2023-03-16T08:06:59Z
I think this must be a medium, all things considered.
#13 - c4-judge
2023-03-19T09:43:31Z
thereksfour marked the issue as selected for report
#14 - c4-judge
2023-03-19T10:07:56Z
thereksfour marked the issue as satisfactory