From a033b01c9520d6de35de163c4245c0aaa5523305 Mon Sep 17 00:00:00 2001 From: Cole Killian Date: Tue, 26 Apr 2022 14:44:02 -0400 Subject: [PATCH] fix: unnecessary execute msg duplication (#55) (oak-glow-v2 finding 19) ## What's This Fix unnecessary duplication of `MigrateOldDepositors` and `UpdateConfig` messages. Also update `migrate_old_depositors` function to throw an error if there are no lotteries left to migrate. * fix: unnecessary duplication of execute msgs Unnecessary duplication of MigrateOldDepositors and UpdateConfig messages --- contracts/lotto/src/contract.rs | 76 +++++++++++---------------------- contracts/lotto/src/state.rs | 20 +++------ contracts/lotto/src/tests.rs | 16 ++++--- 3 files changed, 42 insertions(+), 70 deletions(-) diff --git a/contracts/lotto/src/contract.rs b/contracts/lotto/src/contract.rs index f48cb37..9901d9d 100644 --- a/contracts/lotto/src/contract.rs +++ b/contracts/lotto/src/contract.rs @@ -320,35 +320,7 @@ pub fn execute( ExecuteMsg::ExecuteLottery {} => execute_lottery(deps, env, info), ExecuteMsg::ExecutePrize { limit } => execute_prize(deps, env, info, limit), ExecuteMsg::ExecuteEpochOps {} => execute_epoch_ops(deps, env), - ExecuteMsg::UpdateConfig { - owner, - oracle_addr, - reserve_factor, - instant_withdrawal_fee, - unbonding_period, - epoch_interval, - max_holders, - max_tickets_per_depositor, - paused, - lotto_winner_boost_config, - operator_glow_emission_rate, - sponsor_glow_emission_rate, - } => execute_update_config( - deps, - info, - owner, - oracle_addr, - reserve_factor, - instant_withdrawal_fee, - unbonding_period, - epoch_interval, - max_holders, - max_tickets_per_depositor, - paused, - lotto_winner_boost_config, - operator_glow_emission_rate, - sponsor_glow_emission_rate, - ), + ExecuteMsg::UpdateConfig { .. } => unreachable!(), ExecuteMsg::UpdateLotteryConfig { lottery_interval, block_time, @@ -364,9 +336,7 @@ pub fn execute( prize_distribution, round_delta, ), - ExecuteMsg::MigrateOldDepositors { .. } => Err(ContractError::Std(StdError::generic_err( - "Cannot call MigrateLoop when unpaused.", - ))), + ExecuteMsg::MigrateOldDepositors { .. } => unreachable!(), } } @@ -2140,24 +2110,30 @@ pub fn migrate_old_depositors( // Don't need to include state.current_lottery // because nothing has been saved with id state.current_lottery yet for i in 0..state.current_lottery { - let old_lottery_info = old_read_lottery_info(deps.storage, i); - - let new_lottery_info = LotteryInfo { - rand_round: old_lottery_info.rand_round, - sequence: old_lottery_info.sequence, - awarded: old_lottery_info.awarded, - timestamp: Timestamp::from_seconds(0), - prize_buckets: old_lottery_info.prize_buckets, - number_winners: old_lottery_info.number_winners, - page: old_lottery_info.page, - glow_prize_buckets: [Uint256::zero(); NUM_PRIZE_BUCKETS], - block_height: old_lottery_info.timestamp, - total_user_shares: pool.total_user_shares, - }; - - store_lottery_info(deps.storage, i, &new_lottery_info)?; - - old_remove_lottery_info(deps.storage, i); + let old_lottery_info = old_read_lottery_info(deps.storage, i)?; + + if let Some(old_lottery_info) = old_lottery_info { + let new_lottery_info = LotteryInfo { + rand_round: old_lottery_info.rand_round, + sequence: old_lottery_info.sequence, + awarded: old_lottery_info.awarded, + timestamp: Timestamp::from_seconds(0), + prize_buckets: old_lottery_info.prize_buckets, + number_winners: old_lottery_info.number_winners, + page: old_lottery_info.page, + glow_prize_buckets: [Uint256::zero(); NUM_PRIZE_BUCKETS], + block_height: old_lottery_info.timestamp, + total_user_shares: pool.total_user_shares, + }; + + store_lottery_info(deps.storage, i, &new_lottery_info)?; + + old_remove_lottery_info(deps.storage, i); + } else { + return Err(ContractError::Std(StdError::generic_err( + "Already migrated depositors and lotteries", + ))); + } } // Set paused to false and save diff --git a/contracts/lotto/src/state.rs b/contracts/lotto/src/state.rs index ccc0f59..7bf5185 100644 --- a/contracts/lotto/src/state.rs +++ b/contracts/lotto/src/state.rs @@ -272,7 +272,7 @@ pub struct LotteryInfo { pub total_user_shares: Uint256, } -#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] +#[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] pub struct OldLotteryInfo { pub rand_round: u64, pub sequence: String, @@ -315,19 +315,11 @@ pub fn read_lottery_info(storage: &dyn Storage, lottery_id: u64) -> LotteryInfo } } -pub fn old_read_lottery_info(storage: &dyn Storage, lottery_id: u64) -> OldLotteryInfo { - match bucket_read(storage, OLD_PREFIX_LOTTERY).load(&lottery_id.to_be_bytes()) { - Ok(v) => v, - _ => OldLotteryInfo { - rand_round: 0, - sequence: "".to_string(), - awarded: false, - timestamp: 0, - prize_buckets: [Uint256::zero(); NUM_PRIZE_BUCKETS], - number_winners: [0; NUM_PRIZE_BUCKETS], - page: "".to_string(), - }, - } +pub fn old_read_lottery_info( + storage: &dyn Storage, + lottery_id: u64, +) -> StdResult> { + bucket_read(storage, OLD_PREFIX_LOTTERY).may_load(&lottery_id.to_be_bytes()) } pub fn old_remove_lottery_info(storage: &mut dyn Storage, lottery_id: u64) { diff --git a/contracts/lotto/src/tests.rs b/contracts/lotto/src/tests.rs index 8d32b6b..6e8a2a4 100644 --- a/contracts/lotto/src/tests.rs +++ b/contracts/lotto/src/tests.rs @@ -15,8 +15,9 @@ use crate::state::{ old_read_depositor_info, old_read_lottery_info, old_remove_depositor_info, read_depositor_info, read_depositor_stats_at_height, read_lottery_info, read_lottery_prizes, read_prize, read_sponsor_info, store_depositor_info, store_depositor_stats, Config, DepositorInfo, - DepositorStatsInfo, LotteryInfo, OldConfig, OldDepositorInfo, OldPool, OldState, Pool, - PrizeInfo, State, CONFIG, OLDCONFIG, OLDPOOL, OLDSTATE, OLD_PRIZES, POOL, PRIZES, STATE, + DepositorStatsInfo, LotteryInfo, OldConfig, OldDepositorInfo, OldLotteryInfo, OldPool, + OldState, Pool, PrizeInfo, State, CONFIG, OLDCONFIG, OLDPOOL, OLDSTATE, OLD_PRIZES, POOL, + PRIZES, STATE, }; use crate::test_helpers::{ calculate_lottery_prize_buckets, calculate_prize_buckets, @@ -6366,9 +6367,10 @@ pub fn test_migrate() { // Store some old lotteries for i in 0..old_state.current_lottery { - let mut old_lottery = old_read_lottery_info(deps.as_ref().storage, 100); - - old_lottery.sequence = format!("00000{}", i); + let old_lottery = OldLotteryInfo { + sequence: format!("00000{}", i), + ..Default::default() + }; old_store_lottery_info(deps.as_mut().storage, i, &old_lottery).unwrap(); } @@ -6538,7 +6540,9 @@ pub fn test_migrate() { // New Lottery Info for i in 0..state.current_lottery { - let mut old_lottery = old_read_lottery_info(deps.as_ref().storage, 100); + let mut old_lottery = old_read_lottery_info(deps.as_ref().storage, 100) + .unwrap() + .unwrap(); old_lottery.sequence = format!("00000{}", i);