Lottery SC audit fixes #1765
Draft
Lottery SC audit fixes #1765
Conversation
This file contains 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, |
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, | ||
| ); | ||
| } | ||
| }; |
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.
| match self.status(&lottery_name) { | ||
| Status::Inactive => sc_panic!("Lottery is inactive!"), | ||
| Status::Running => sc_panic!("Lottery is still running!"), | ||
| Status::Ended => { | ||
| self.distribute_prizes(&lottery_name); | ||
| self.clear_storage(&lottery_name); | ||
| if self.total_winning_tickets(&lottery_name).is_empty() { |
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.
| @@ -266,7 +262,8 @@ pub trait Lottery { | |||
| self.send().esdt_local_burn(&esdt_token_id, 0, &burn_amount); | |||
| } | |||
|
|
|||
| info.prize_pool -= burn_amount; | |||
| info.prize_pool -= &burn_amount; | |||
| info.unawarded_amount -= burn_amount; | |||
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; |
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]; |
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)), |
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)), |
There was a problem hiding this comment.
Nvm, that's a ManagedVec, starts from 0.
Coverage SummaryTotals
FilesExpand
|
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