Lottery SC audit fixes #1765
Open
Lottery SC audit fixes #1765
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dorin-iancu
reviewed
Sep 25, 2024
| fn accumulated_rewards( | ||
| &self, | ||
| token_id: &EgldOrEsdtTokenIdentifier, | ||
| user: &ManagedAddress, |
Contributor
There was a problem hiding this comment.
To prevent keys being over 32 bytes, use AddressToIdMapper and only use the user's ID here and in user_accumulated_token_rewards mapper.
Additionally, consuder some shorter base storage keys, i.e.
accRew
totalWinTick,
idxLastWin
userAccRew -> btw, don't use the same name for 2 mappers, even if it's theoretically safe for now, someone might modify the code and break everything
nrEntries
burnPerc
Comment on lines
371
to
402
| if tokens.is_empty() { | ||
| // if wanted tokens were not specified claim all, and clear user_accumulated_token_rewards storage mapper | ||
|
|
||
| for token_id in self.user_accumulated_token_rewards(&caller).iter() { | ||
| require!( | ||
| !self.accumulated_rewards(&token_id, &caller).is_empty(), | ||
| "Token requested not available for claim" | ||
| ); | ||
|
|
||
| self.prepare_token_for_claim( | ||
| token_id, | ||
| &caller, | ||
| &mut accumulated_egld_rewards, | ||
| &mut accumulated_esdt_rewards, | ||
| ); | ||
| } | ||
| self.user_accumulated_token_rewards(&caller).clear(); | ||
| } else { | ||
| // otherwise claim just what was requested and remove those tokens from the user_accumulated_token_rewards storage mapper | ||
| for token_id in tokens { | ||
| let _ = &self | ||
| .user_accumulated_token_rewards(&caller) | ||
| .swap_remove(&token_id); | ||
|
|
||
| self.prepare_token_for_claim( | ||
| token_id, | ||
| &caller, | ||
| &mut accumulated_egld_rewards, | ||
| &mut accumulated_esdt_rewards, | ||
| ); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
This function is way too big. Also, you can get rid of the duplicated code by first reading all the storage and putting all values in a managed vec.
Suggested change
| if tokens.is_empty() { | |
| // if wanted tokens were not specified claim all, and clear user_accumulated_token_rewards storage mapper | |
| for token_id in self.user_accumulated_token_rewards(&caller).iter() { | |
| require!( | |
| !self.accumulated_rewards(&token_id, &caller).is_empty(), | |
| "Token requested not available for claim" | |
| ); | |
| self.prepare_token_for_claim( | |
| token_id, | |
| &caller, | |
| &mut accumulated_egld_rewards, | |
| &mut accumulated_esdt_rewards, | |
| ); | |
| } | |
| self.user_accumulated_token_rewards(&caller).clear(); | |
| } else { | |
| // otherwise claim just what was requested and remove those tokens from the user_accumulated_token_rewards storage mapper | |
| for token_id in tokens { | |
| let _ = &self | |
| .user_accumulated_token_rewards(&caller) | |
| .swap_remove(&token_id); | |
| self.prepare_token_for_claim( | |
| token_id, | |
| &caller, | |
| &mut accumulated_egld_rewards, | |
| &mut accumulated_esdt_rewards, | |
| ); | |
| } | |
| }; | |
| if tokens.is_empty() { | |
| let mut all_tokens = ManagedVec::new(); | |
| for token_id in self.user_accumulated_token_rewards(&caller).iter() { | |
| all_tokens.push(token_id); | |
| } | |
| self.claim_rewards_user(&self, all_tokens, ...); | |
| } else { | |
| self.claim_rewards_user(&self, tokens.into(), ...); | |
| }; |
Something like this. Much cleaner and easier to understand. And you do whatever cleanup you have to do in the claim_rewards_user function.
If you want to go for max efficiency, try passing only an iterator to the claim_rewards_user function instead of the ManagedVec itself, and in the case the user gave empty vec as argument, give all_tokens.rev() as argument. The swap_remove operation will be more efficient.
| Status::Ended => { | ||
| self.distribute_prizes(&lottery_name); | ||
| self.clear_storage(&lottery_name); | ||
| if self.total_winning_tickets(&lottery_name).is_empty() { |
Contributor
There was a problem hiding this comment.
Maybe add a check for lottery_info(&lottery_name).is_empty(). Sure, for now it will crash anyway when you try a get(), but it's safer this way.
|
|
||
| info.prize_pool -= burn_amount; | ||
| info.prize_pool -= &burn_amount; | ||
| info.unawarded_amount -= burn_amount; |
Contributor
There was a problem hiding this comment.
Can't comment on unmodified lines, but you should move this whole burn logic to its own function.
And once you do that, you can also de-nest the code by:
if burn_percentage == 0 {
return;
}
// other code here
Comment on lines
297
to
336
| let rand_index = self.get_distinct_random(index_last_winner, total_tickets, 1)[0]; | ||
|
|
||
| // swap indexes of the winner addresses - we are basically bringing the winners in the first indexes of the mapper | ||
| let winner_address = self.ticket_holders(lottery_name).get(rand_index).clone(); | ||
| let last_index_winner_address = | ||
| self.ticket_holders(lottery_name).get(index_last_winner); | ||
| self.ticket_holders(lottery_name) | ||
| .set(rand_index, &last_index_winner_address); | ||
| self.ticket_holders(lottery_name) | ||
| .set(index_last_winner, &winner_address); | ||
|
|
||
| // distribute to the first place last. Laws of probability say that order doesn't matter. | ||
| // this is done to mitigate the effects of BigUint division leading to "spare" prize money being left out at times | ||
| // 1st place will get the spare money instead. | ||
| if index_last_winner < total_winning_tickets { | ||
| let prize = self.calculate_percentage_of( | ||
| &info.prize_pool, | ||
| &BigUint::from(info.prize_distribution.get(index_last_winner)), | ||
| ); | ||
| if prize > 0 { | ||
| self.assign_prize_to_winner( | ||
| info.token_identifier.clone(), | ||
| &prize, | ||
| &winner_address, | ||
| ); | ||
|
|
||
| info.unawarded_amount -= prize; | ||
| } | ||
| } else { | ||
| // insert token in accumulated rewards first place | ||
| let first_place_winner = ticket_holders_mapper.get(index_last_winner); | ||
|
|
||
| self.assign_prize_to_winner( | ||
| info.token_identifier.clone(), | ||
| &info.unawarded_amount, | ||
| &first_place_winner, | ||
| ); | ||
| } | ||
| index_last_winner += 1; | ||
| iterations += 1; |
Contributor
There was a problem hiding this comment.
Really should be its own function.
|
|
||
| let mut iterations = 0; | ||
| while index_last_winner <= total_winning_tickets && iterations < MAX_OPERATIONS { | ||
| let rand_index = self.get_distinct_random(index_last_winner, total_tickets, 1)[0]; |
Contributor
There was a problem hiding this comment.
This way of generating random numbers is terribly inefficient if all you want is one number. If this is the only place where you use that function, simply rewrite it to generate one number in range (index_last_winner, total_tickets)
| if index_last_winner < total_winning_tickets { | ||
| let prize = self.calculate_percentage_of( | ||
| &info.prize_pool, | ||
| &BigUint::from(info.prize_distribution.get(index_last_winner)), |
Contributor
There was a problem hiding this comment.
I think it should be this way, since you say you'll do the distribution for the first place last.
Suggested change
| &BigUint::from(info.prize_distribution.get(index_last_winner)), | |
| &BigUint::from(info.prize_distribution.get(index_last_winner + 1)), |
Contributor
There was a problem hiding this comment.
Nvm, that's a ManagedVec, starts from 0.
Coverage SummaryTotals
FilesExpand
|
|
Contract comparison - from 87227c5 to 9fcc9cb
|
dorin-iancu
previously approved these changes
Nov 18, 2025
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements fixes from a security audit for the lottery-esdt smart contract. The changes significantly refactor the contract architecture and address several security and functionality concerns identified in the audit.
Key Changes:
- Refactored contract into modular structure (basics/ and specific/ modules) for better organization
- Changed token support from EGLD/ESDT to ESDT-only (TokenIdentifier instead of EgldOrEsdtTokenIdentifier)
- Implemented two-phase prize claiming mechanism: prizes accumulate in storage, users explicitly claim rewards
- Introduced address-to-ID mapping for storage optimization
- Made determine_winner callable multiple times with MAX_OPERATIONS limit per call for better gas management
- Renamed endpoint from "start"/"createLotteryPool" to "startLottery" for consistency
- Added shard validation requirement for determine_winner calls
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/lottery.rs |
Complete refactor from monolithic to modular trait composition architecture |
src/specific/award.rs |
New awarding module with batched prize distribution and accumulated rewards pattern |
src/specific/claim.rs |
New claim module allowing users to withdraw accumulated rewards |
src/specific/buy.rs |
Extracted ticket purchase logic with address-to-ID mapper integration |
src/specific/setup.rs |
Extracted lottery setup logic, enforces ESDT-only tokens |
src/basics/storage.rs |
Centralized storage mappers using u64 IDs instead of addresses |
src/basics/utils.rs |
Refactored random selection from array-based to single random value |
src/lottery_proxy.rs |
Auto-generated proxy file (has type mismatches with actual contract) |
scenarios/*.scen.json |
Updated test scenarios to reflect new storage patterns and shard requirements |
wasm/src/lib.rs |
Updated endpoint list order and naming |
tests/*.rs |
Added ignore attributes for tests requiring shard simulation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
contracts/examples/lottery-esdt/scenarios/buy-ticket-all-options.scen.json
Show resolved
Hide resolved
| info.unawarded_amount -= prize; | ||
| } else { | ||
| // insert token in accumulated rewards first place | ||
| let first_place_winner = ticket_holders_mapper.get(*index_last_winner); |
There was a problem hiding this comment.
Using index_last_winner to access ticket_holders_mapper.get(*index_last_winner) when *index_last_winner > total_winning_tickets could be accessing an index that has already been used in a previous iteration. Since the first place winner should receive any leftover unawarded_amount, the correct index should be 1 (the first winner), not *index_last_winner. This bug would result in giving leftover funds to the wrong winner.
| total_winning_tickets: usize, | ||
| info: &mut LotteryInfo<Self::Api>, | ||
| ) { | ||
| let rand_index = self.get_distinct_random(*index_last_winner, total_tickets); |
There was a problem hiding this comment.
The function get_distinct_random is called with parameters (min: *index_last_winner, max: total_tickets) but based on the usage pattern and the need to select a random ticket holder, this should likely be called with (min: *index_last_winner, max: total_tickets + 1) or the implementation of get_distinct_random needs to be verified for inclusive/exclusive range handling. The old implementation returned ArrayVec with shuffled indices, but the new implementation just returns a single random value. Verify that the random selection correctly covers the intended range.
Comment on lines
+15
to
+16
| #[storage_mapper("totalWinning_tickets")] | ||
| fn total_winning_tickets(&self, lottery_name: &ManagedBuffer) -> SingleValueMapper<usize>; |
There was a problem hiding this comment.
The storage mapper key has an inconsistent naming format: "totalWinning_tickets" uses an underscore between words, while other mappers use camelCase (e.g., "ticketHolder", "indexLastWinner"). This inconsistency could lead to confusion. Consider using "totalWinningTickets" for consistency with the rest of the codebase.
Comment on lines
+158
to
+182
| if *index_last_winner <= total_winning_tickets { | ||
| let prize = self.calculate_percentage_of( | ||
| &info.prize_pool, | ||
| &BigUint::from( | ||
| info.prize_distribution | ||
| .get(total_winning_tickets - *index_last_winner), | ||
| ), | ||
| ); | ||
| if prize == 0 { | ||
| return; | ||
| } | ||
|
|
||
| self.assign_prize_to_winner(info.token_identifier.clone(), &prize, &winner_address); | ||
|
|
||
| info.unawarded_amount -= prize; | ||
| } else { | ||
| // insert token in accumulated rewards first place | ||
| let first_place_winner = ticket_holders_mapper.get(*index_last_winner); | ||
|
|
||
| self.assign_prize_to_winner( | ||
| info.token_identifier.clone(), | ||
| &info.unawarded_amount, | ||
| &first_place_winner, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The condition 'if *index_last_winner <= total_winning_tickets' will always be true within the award_winner function because the calling loop at line 98 only invokes award_winner when this condition holds. The else branch (lines 173-182) is unreachable dead code. This suggests a logic error in how the first place winner receives leftover unawarded_amount. Consider moving the leftover distribution logic outside the loop or restructuring to execute after all winners are processed.
Comment on lines
+44
to
+47
| require!( | ||
| !self.accumulated_rewards(&token_id, caller_id).is_empty(), | ||
| "Token requested not available for claim" | ||
| ); |
There was a problem hiding this comment.
The error message "Token requested not available for claim" is misleading in this context. This error is triggered in handle_claim_with_unspecified_tokens when the user didn't specify any tokens (claiming all). A more accurate message would be "Internal error: token in reward set has no balance" or the check should be removed since user_accumulated_token_rewards should only contain tokens with non-empty balances.
| caller_id: &u64, | ||
| accumulated_rewards: &mut MultiEsdtPayment<Self::Api>, | ||
| ) { | ||
| for token_id in tokens.iter().rev() { |
There was a problem hiding this comment.
Iterating tokens in reverse order with .rev() is unnecessary here and may cause confusion. The order of iteration doesn't affect the correctness of the claim operation, and reversing adds unnecessary complexity. Consider removing .rev() for clearer code unless there's a specific reason for reverse iteration.
dorin-iancu
approved these changes
Dec 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes based on the following audit: https://docs.google.com/document/d/1ky-H_dGx3oPRSQkP83UKzkZRXl9V-SOc5cSvr8hq-Ow/edit#heading=h.bgyek48ody10