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

1 stars 1 forks source link

Minimum referral requirement is incorrectly computed #387

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

As per the docs:

Referrer rewards are allocated on a per draw basis
Referrers who meet the minimum referral requirement according to the following table will be eligible for the Individual Referrer Allocation

The minimum referral requirement for a draw N + 1 is computed based on the number of tickets sold for the draw N.

In the code, this is computed upon draw execution of the draw N. In receiveRandomNumber():

src/Lottery.sol
224:         winningTicket[drawFinalized] = _winningTicket;
225:         drawExecutionInProgress = false;
226: 
227:         uint256 ticketsSoldDuringDraw = nextTicketId - lastDrawFinalTicketId;
228:         lastDrawFinalTicketId = nextTicketId;
229:         // solhint-disable-next-line max-line-length
230:         referralDrawFinalize(drawFinalized, ticketsSoldDuringDraw);

The number used as "number of tickets sold for the draw N" is ticketsSoldDuringDraw. It is computed as nextTicketId - lastDrawFinalTicketId.

The issue is that nextTicketId is the id of the last Ticket minted. But tickets are minted for every draw, and ticketId is incremented on mint(), regardless of the drawId

src/Ticket.sol
23:     function mint(address to, uint128 drawId, uint120 combination) internal returns (uint256 ticketId) {
24:         ticketId = nextTicketId++; //@audit - not a function of drawId
25:         ticketsInfo[ticketId] = TicketInfo(drawId, combination, false);
26:         _mint(to, ticketId);
27:     }

This means ticketsSoldDuringDraw accounts for tickets sold during the draw N, not for it.

Impact

The minimum referral requirement maths is completely broken, and will lead to a potential loss of rewards for referrers.

Proof of Concept

let us look at a draw N. Before execution, lastDrawFinalTicketId == X. Execution now happens. Between the last draw execution and this one, there was a specifically high number of ticket sales, for draws ranging from N to M: assume 5,000 were sold for draw N, but 50,000 were sold in total.

ticketsSoldDuringDraw = nextTicketId - lastDrawFinalTicketId = 50,000

The minimum referral amount is computed:

src/ReferralSystem.sol
93:         minimumEligibleReferrals[drawFinalized + 1] =
94:             getMinimumEligibleReferralsFactorCalculation(ticketsSoldDuringDraw);

Because 50,000 tickets were sold, the minimum number of referrals for draw N + 1 will be 0,75% 50,000 = 375, while it should have been 5,000 1% = 50. Referrers for draw N + 1 will need to find 325 more referrers than what they should.

Some referrers will lose on the reward they were entitled to (if their referral amount is between 50 and 375).

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use a mapping for nextTicketId in Ticket, so that it accounts for the drawId

c4-judge commented 1 year ago

thereksfour marked the issue as primary issue

d3e4 commented 1 year ago

"This means ticketsSoldDuringDraw accounts for tickets sold during the draw N, not for it." Note the "during" in the variable name. I think this is indeed the intention, that players are referred to the lottery (a ticket purchase) rather than to specific draws.

c4-sponsor commented 1 year ago

TutaRicky marked the issue as sponsor disputed

TutaRicky commented 1 year ago

Warden didn't understand referral requirement calculation

c4-judge commented 1 year ago

thereksfour marked the issue as unsatisfactory: Invalid