From aa9ad0f4a49c4e8ffe07d0e30037d01af07ffd9d Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 18:31:24 +0200 Subject: [PATCH 1/8] Add overflow checks to initialization --- src/lib.rs | 11 ++++++----- src/tests.rs | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9e5b6c2..cde10ea 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -412,7 +412,7 @@ pub mod pallet { let incoming_rewards: BalanceOf = rewards .iter() .fold(0u32.into(), |acc: BalanceOf, (_, _, reward)| { - acc + *reward + acc.saturating_add(*reward) }); // Ensure we dont go over funds @@ -470,19 +470,20 @@ pub mod pallet { claimed_reward: initial_payment, }; + // It should be safe not to use saturating_add here, as we already checked before that these rewards do not overflow existing ones current_initialized_rewards += *reward - initial_payment; total_contributors += 1; + if let Some(native_account) = native_account { if let Some(inserted_reward_info) = AccountsPayable::::get(native_account) { // the native account has already some rewards in, we add the new ones AccountsPayable::::insert( native_account, + // It should be safe not to use saturating_add here, as we already checked before that these rewards do not overflow existing ones RewardInfo { - total_reward: inserted_reward_info.total_reward - + reward_info.total_reward, - claimed_reward: inserted_reward_info.claimed_reward - + reward_info.claimed_reward, + total_reward: inserted_reward_info.total_reward + reward_info.total_reward, + claimed_reward: inserted_reward_info.claimed_reward + reward_info.claimed_reward, }, ); } else { diff --git a/src/tests.rs b/src/tests.rs index 78bdd02..344e24a 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -877,3 +877,27 @@ fn test_initialization_errors() { ); }); } + + +#[test] +fn test_assert_we_cannot_overflow_at_init() { + empty().execute_with(|| { + // The init relay block gets inserted + roll_to(2); + let init_block = Crowdloan::init_relay_block(); + + let pot = Crowdloan::pot(); + + // Too many contributors + assert_noop!( + Crowdloan::initialize_reward_vec( + Origin::root(), + vec![ + ([1u8; 32].into(), Some(1), 1), + ([2u8; 32].into(), Some(2), u128::MAX), + ] + ), + Error::::TooManyContributors + ); + }); +} \ No newline at end of file From f02e54739884f13f109f7f59fceffb64dec128f9 Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 18:38:26 +0200 Subject: [PATCH 2/8] Add test --- src/tests.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/tests.rs b/src/tests.rs index 344e24a..e3b36ad 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -884,10 +884,6 @@ fn test_assert_we_cannot_overflow_at_init() { empty().execute_with(|| { // The init relay block gets inserted roll_to(2); - let init_block = Crowdloan::init_relay_block(); - - let pot = Crowdloan::pot(); - // Too many contributors assert_noop!( Crowdloan::initialize_reward_vec( @@ -897,7 +893,7 @@ fn test_assert_we_cannot_overflow_at_init() { ([2u8; 32].into(), Some(2), u128::MAX), ] ), - Error::::TooManyContributors + Error::::BatchBeyondFundPot ); }); } \ No newline at end of file From ea2302133022f230f605b0c126a49442a6062385 Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 18:42:45 +0200 Subject: [PATCH 3/8] Fmt --- src/lib.rs | 7 ++++--- src/tests.rs | 3 +-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index cde10ea..19a98b8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -474,7 +474,6 @@ pub mod pallet { current_initialized_rewards += *reward - initial_payment; total_contributors += 1; - if let Some(native_account) = native_account { if let Some(inserted_reward_info) = AccountsPayable::::get(native_account) { // the native account has already some rewards in, we add the new ones @@ -482,8 +481,10 @@ pub mod pallet { native_account, // It should be safe not to use saturating_add here, as we already checked before that these rewards do not overflow existing ones RewardInfo { - total_reward: inserted_reward_info.total_reward + reward_info.total_reward, - claimed_reward: inserted_reward_info.claimed_reward + reward_info.claimed_reward, + total_reward: inserted_reward_info.total_reward + + reward_info.total_reward, + claimed_reward: inserted_reward_info.claimed_reward + + reward_info.claimed_reward, }, ); } else { diff --git a/src/tests.rs b/src/tests.rs index e3b36ad..41bd9a8 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -878,7 +878,6 @@ fn test_initialization_errors() { }); } - #[test] fn test_assert_we_cannot_overflow_at_init() { empty().execute_with(|| { @@ -896,4 +895,4 @@ fn test_assert_we_cannot_overflow_at_init() { Error::::BatchBeyondFundPot ); }); -} \ No newline at end of file +} From bb6f43b5be164945af5fde0490703b0b776b5b36 Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 18:49:46 +0200 Subject: [PATCH 4/8] Add a further check --- src/lib.rs | 2 +- src/tests.rs | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 19a98b8..bed4122 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -417,7 +417,7 @@ pub mod pallet { // Ensure we dont go over funds ensure!( - current_initialized_rewards + incoming_rewards <= Self::pot(), + current_initialized_rewards.saturating_add(incoming_rewards) <= Self::pot(), Error::::BatchBeyondFundPot ); diff --git a/src/tests.rs b/src/tests.rs index 41bd9a8..5127dec 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -883,13 +883,17 @@ fn test_assert_we_cannot_overflow_at_init() { empty().execute_with(|| { // The init relay block gets inserted roll_to(2); - // Too many contributors + assert_ok!(Crowdloan::initialize_reward_vec( + Origin::root(), + vec![([1u8; 32].into(), Some(1), 500u32.into()),] + )); + // This should overflow assert_noop!( Crowdloan::initialize_reward_vec( Origin::root(), vec![ - ([1u8; 32].into(), Some(1), 1), - ([2u8; 32].into(), Some(2), u128::MAX), + ([2u8; 32].into(), Some(1), 1), + ([3u8; 32].into(), Some(2), u128::MAX), ] ), Error::::BatchBeyondFundPot From 605fab4dd6bced19e1525010d0cc0681d92f6896 Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 18:51:24 +0200 Subject: [PATCH 5/8] Comment fmt --- src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index bed4122..d5a6e29 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -470,7 +470,8 @@ pub mod pallet { claimed_reward: initial_payment, }; - // It should be safe not to use saturating_add here, as we already checked before that these rewards do not overflow existing ones + // It should be safe not to use saturating_add here + // as we already checked before that these rewards do not overflow existing ones current_initialized_rewards += *reward - initial_payment; total_contributors += 1; @@ -479,7 +480,8 @@ pub mod pallet { // the native account has already some rewards in, we add the new ones AccountsPayable::::insert( native_account, - // It should be safe not to use saturating_add here, as we already checked before that these rewards do not overflow existing ones + // It should be safe not to use saturating_add here + // as we already checked before that these rewards do not overflow existing ones RewardInfo { total_reward: inserted_reward_info.total_reward + reward_info.total_reward, From e2eb006cd6db28638345bfced9bb7be991fd1a69 Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 18:52:14 +0200 Subject: [PATCH 6/8] fix test --- src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests.rs b/src/tests.rs index 5127dec..f5e48dd 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -892,8 +892,8 @@ fn test_assert_we_cannot_overflow_at_init() { Crowdloan::initialize_reward_vec( Origin::root(), vec![ - ([2u8; 32].into(), Some(1), 1), - ([3u8; 32].into(), Some(2), u128::MAX), + ([2u8; 32].into(), Some(2), 1), + ([3u8; 32].into(), Some(3), u128::MAX), ] ), Error::::BatchBeyondFundPot From 726550b07d4cafdfb3db8472f74dfc1d964f0ecc Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 19:15:49 +0200 Subject: [PATCH 7/8] Add it also when summing to previous rewards --- src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d5a6e29..be07672 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -189,8 +189,8 @@ pub mod pallet { ensure!( proof.verify(payload.as_slice(), &relay_account.clone().into()), Error::::InvalidClaimSignature + ); - // We ensure the relay chain id wast not yet associated to avoid multi-claiming ensure!( ClaimedRelayChainIds::::get(&relay_account).is_none(), @@ -484,9 +484,9 @@ pub mod pallet { // as we already checked before that these rewards do not overflow existing ones RewardInfo { total_reward: inserted_reward_info.total_reward - + reward_info.total_reward, + .saturating_add(reward_info.total_reward), claimed_reward: inserted_reward_info.claimed_reward - + reward_info.claimed_reward, + .saturating_add(reward_info.claimed_reward), }, ); } else { From 01d141e936b2ceeda182a4fe4259f9b71a73a370 Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 17 Aug 2021 19:18:06 +0200 Subject: [PATCH 8/8] Clean comments --- src/lib.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index be07672..c59950e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -469,19 +469,15 @@ pub mod pallet { total_reward: *reward, claimed_reward: initial_payment, }; - - // It should be safe not to use saturating_add here - // as we already checked before that these rewards do not overflow existing ones - current_initialized_rewards += *reward - initial_payment; - total_contributors += 1; + + current_initialized_rewards = current_initialized_rewards.saturating_add((*reward).saturating_sub(initial_payment)); + total_contributors = total_contributors.saturating_add(1); if let Some(native_account) = native_account { if let Some(inserted_reward_info) = AccountsPayable::::get(native_account) { // the native account has already some rewards in, we add the new ones AccountsPayable::::insert( native_account, - // It should be safe not to use saturating_add here - // as we already checked before that these rewards do not overflow existing ones RewardInfo { total_reward: inserted_reward_info.total_reward .saturating_add(reward_info.total_reward),