Merge pull request #42 from hashed-io/fix/unsafe-arithmetics

Fix/unsafe arithmetics

Author: sebastianmontero

pallets/afloat/src/functions.rs

@@ -4,16 +4,16 @@ use frame_support::pallet_prelude::*;
 use frame_system::{pallet_prelude::*, RawOrigin};
 use pallet_fruniques::types::{Attributes, CollectionDescription, FruniqueRole, ParentInfo};
 use pallet_gated_marketplace::types::{Marketplace, MarketplaceRole};
-use sp_runtime::{traits::StaticLookup, Permill};
 // use frame_support::traits::OriginTrait;
 use core::convert::TryInto;
 use frame_support::traits::Time;
 use pallet_rbac::types::{IdOrVec, RoleBasedAccessControl, RoleId};
 use scale_info::prelude::vec;
 use sp_io::hashing::blake2_256;
 use sp_runtime::{
+	Permill,
 	sp_std::{str, vec::Vec},
-	traits::Zero,
+	traits::{Zero, StaticLookup, CheckedMul},
 };
 
 impl<T: Config> Pallet<T> {
@@ -473,19 +473,19 @@ impl<T: Config> Pallet<T> {
 			offer.tax_credit_amount_remaining >= tax_credit_amount,
 			Error::<T>::NotEnoughTaxCreditsAvailable
 		);
+
+		let price_per_credit: T::Balance = offer.price_per_credit.into();
+		let total_price: T::Balance = Self::safe_multiply_offer(price_per_credit, tax_credit_amount)?;
 		//ensure user has enough afloat balance
 		ensure!(
-			Self::do_get_afloat_balance(who.clone())? >=
-				offer.price_per_credit * tax_credit_amount.into(),
+			Self::do_get_afloat_balance(who.clone())? >= total_price,
 			Error::<T>::NotEnoughAfloatBalanceAvailable
 		);
 		let zero_balance: T::Balance = Zero::zero();
 		//ensure tax credit amount is greater than zero
 		ensure!(tax_credit_amount > zero_balance, Error::<T>::Underflow);
 
 		let creation_date: u64 = T::Timestamp::now().into();
-		let price_per_credit: T::Balance = offer.price_per_credit.into();
-		let total_price: T::Balance = price_per_credit * tax_credit_amount;
 		let fee: Option<T::Balance> = None;
 		let tax_credit_id: <T as pallet_uniques::Config>::ItemId = offer.tax_credit_id;
 		let seller_id: T::AccountId = offer.creator_id;
@@ -531,6 +531,8 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
+	
+
 	/// Confirms a sell transaction.
 	///
 	/// # Arguments
@@ -933,6 +935,14 @@ impl<T: Config> Pallet<T> {
 			})
 	}
 
+	fn safe_multiply_offer(
+		factor1: T::Balance,
+		factor2: T::Balance
+	) -> Result<T::Balance, DispatchError> {
+		let result = factor1.checked_mul(&factor2).ok_or_else(|| Error::<T>::ArithmeticOverflow)?;
+		Ok(result)
+	}
+
 	fn get_all_roles_for_user(account_id: T::AccountId) -> Result<Vec<AfloatRole>, DispatchError> {
 		let roles_storage = <T as pallet::Config>::Rbac::get_roles_by_user(
 			account_id.clone(),

pallets/afloat/src/lib.rs

@@ -146,6 +146,8 @@ pub mod pallet {
 		ChildOfferIdNotFound,
 		// Tax credit amount underflow
 		Underflow,
+		// Arithmetic multiplication overflow
+		ArithmeticOverflow,
 		// Afloat marketplace label too long
 		LabelTooLong,
 		// Afloat asset has not been set

pallets/afloat/src/mock.rs

@@ -187,6 +187,7 @@ impl pallet_rbac::Config for Test {
 	type MaxPermissionsPerRole = MaxPermissionsPerRole;
 	type MaxRolesPerUser = MaxRolesPerUser;
 	type MaxUsersPerRole = MaxUsersPerRole;
+	type WeightInfo = ();
 }
 
 impl pallet_timestamp::Config for Test {

pallets/afloat/src/tests.rs

@@ -1,5 +1,5 @@
 use super::*;
-use crate::{mock::*, types::*, Error};
+use crate::{mock::*, types::*, AfloatOffers, Error};
 use frame_support::{assert_noop, assert_ok, traits::Currency, BoundedVec};
 use frame_system::RawOrigin;
 
@@ -16,6 +16,61 @@ fn dummy_description() -> BoundedVec<u8, StringLimit> {
 //buy_fee = 2%
 //sell_fee = 4%
 
+#[test]
+fn replicate_overflow_for_start_take_sell_order() {
+	TestExternalitiesBuilder::new().initialize_all().build().execute_with(|| {
+		let user = new_account(3);
+		let other_user = new_account(4);
+		let item_id = 0;
+
+		Balances::make_free_balance_be(&user, 100);
+		Balances::make_free_balance_be(&other_user, 100);
+
+		let args = SignUpArgs::BuyerOrSeller {
+			cid: ShortString::try_from(b"cid".to_vec()).unwrap(),
+			cid_creator: ShortString::try_from(b"cid_creator".to_vec()).unwrap(),
+			group: ShortString::try_from(b"Group".to_vec()).unwrap(),
+		};
+
+		assert_ok!(Afloat::sign_up(RawOrigin::Signed(user.clone()).into(), args.clone()));
+		assert_ok!(Afloat::sign_up(RawOrigin::Signed(other_user.clone()).into(), args.clone()));
+
+		assert_ok!(Afloat::set_afloat_balance(
+			RuntimeOrigin::signed(1),
+			other_user.clone(),
+			100000
+		));
+		assert_eq!(Afloat::do_get_afloat_balance(other_user.clone()).unwrap(), 100000);
+
+		assert_ok!(Afloat::create_tax_credit(
+			RawOrigin::Signed(user.clone()).into(),
+			dummy_description(),
+			None,
+			None,
+		));
+
+		assert_ok!(Afloat::create_offer(
+			RawOrigin::Signed(user.clone()).into(),
+			CreateOfferArgs::Sell {
+				tax_credit_id: item_id,
+				price_per_credit: 18446744073709551615,
+				tax_credit_amount: 10,
+				expiration_date: 1000
+			}
+		));
+
+		let (offer_id, _offer) = AfloatOffers::<Test>::iter().next().unwrap().clone();
+		assert_noop!(
+			Afloat::start_take_sell_order(
+				RawOrigin::Signed(other_user.clone()).into(),
+				offer_id,
+				10
+			),
+			Error::<Test>::ArithmeticOverflow
+		);
+	});
+}
+
 #[test]
 fn sign_up_works() {
 	TestExternalitiesBuilder::new().initialize_all().build().execute_with(|| {

pallets/fund-admin/src/functions.rs

@@ -2008,8 +2008,8 @@ impl<T: Config> Pallet<T> {
 
 		// Create revenue transaction id
 		let revenue_transaction_id =
-			(revenue_id, job_eligible_id, project_id, timestamp).using_encoded(blake2_256);
-
+			(revenue_id, revenue_amount, job_eligible_id, project_id, timestamp)
+				.using_encoded(blake2_256);
 		// Ensure revenue transaction id doesn't exist
 		ensure!(
 			!RevenueTransactionsInfo::<T>::contains_key(revenue_transaction_id),
@@ -2031,11 +2031,6 @@ impl<T: Config> Pallet<T> {
 		};
 
 		// Insert revenue transaction data into RevenueTransactionsInfo
-		// Ensure revenue transaction id doesn't exist
-		ensure!(
-			!RevenueTransactionsInfo::<T>::contains_key(revenue_transaction_id),
-			Error::<T>::RevenueTransactionIdAlreadyExists
-		);
 		<RevenueTransactionsInfo<T>>::insert(revenue_transaction_id, revenue_transaction_data);
 
 		// Insert revenue transaction id into TransactionsByRevenue
@@ -3125,6 +3120,14 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
+	fn safe_add(
+		a: u64,
+		b: u64
+	) -> Result<u64, DispatchError> {
+		let result = a.checked_add(b).ok_or_else(|| Error::<T>::ArithmeticOverflow)?;
+		Ok(result)
+	}
+
 	fn do_calculate_drawdown_total_amount(
 		project_id: [u8; 32],
 		drawdown_id: [u8; 32],
@@ -3147,7 +3150,7 @@ impl<T: Config> Pallet<T> {
 					.ok_or(Error::<T>::TransactionNotFound)?;
 
 				// Add transaction amount to drawdown total amount
-				drawdown_total_amount += transaction_data.amount;
+				drawdown_total_amount = Self::safe_add(drawdown_total_amount, transaction_data.amount)?;
 			}
 		}
 
@@ -3299,7 +3302,7 @@ impl<T: Config> Pallet<T> {
 						.ok_or(Error::<T>::RevenueTransactionNotFound)?;
 
 				// Add revenue transaction amount to revenue total amount
-				revenue_total_amount += revenue_transaction_data.amount;
+				revenue_total_amount = Self::safe_add(revenue_total_amount, revenue_transaction_data.amount)?;
 			}
 		}
 

pallets/fund-admin/src/lib.rs

@@ -694,6 +694,8 @@ pub mod pallet {
 		AdminHasNoFreeBalance,
 		/// Administrator account has insuficiente balance to register a new user
 		InsufficientFundsToTransfer,
+		// Arithmetic addition overflow
+		ArithmeticOverflow,
 	}
 
 	// E X T R I N S I C S

pallets/fund-admin/src/mock.rs

@@ -152,6 +152,7 @@ impl pallet_rbac::Config for Test {
 	type MaxRolesPerUser = MaxRolesPerUser;
 	type MaxUsersPerRole = MaxUsersPerRole;
 	type RemoveOrigin = EnsureRoot<Self::AccountId>;
+	type WeightInfo = ();
 }
 
 // Build genesis storage according to the mock runtime.

pallets/fund-admin/src/tests.rs

@@ -4174,6 +4174,50 @@ fn drawdowns_a_user_submits_a_drawdown_for_approval_works() {
 	});
 }
 
+#[test]
+fn replicate_overflow_for_a_drawdown_submission() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(make_default_full_project());
+		let project_id = ProjectsInfo::<Test>::iter_keys().next().unwrap();
+
+		let drawdown_id = get_drawdown_id(project_id, DrawdownType::EB5, 1);
+		let expenditure_id = get_budget_expenditure_id(
+			project_id,
+			make_field_name("Expenditure Test 1"),
+			ExpenditureType::HardCost,
+		);
+
+		let transaction_data =
+			make_transaction(Some(expenditure_id), Some(1000), CUDAction::Create, None);
+
+		assert_ok!(FundAdmin::submit_drawdown(
+			RuntimeOrigin::signed(2),
+			project_id,
+			drawdown_id,
+			Some(transaction_data),
+			false,
+		));
+
+		let second_transaction_data = make_transaction(
+			Some(expenditure_id),
+			Some(18446744073709551615),
+			CUDAction::Create,
+			None,
+		);
+
+		assert_noop!(
+			FundAdmin::submit_drawdown(
+				RuntimeOrigin::signed(2),
+				project_id,
+				drawdown_id,
+				Some(second_transaction_data),
+				true,
+			),
+			Error::<Test>::ArithmeticOverflow
+		);
+	});
+}
+
 #[test]
 fn drawdowns_a_user_submits_a_draft_drawdown_for_approval_works() {
 	new_test_ext().execute_with(|| {
@@ -5790,6 +5834,46 @@ fn revenues_after_a_revenue_is_submitted_the_next_one_is_generated_automaticaly_
 	});
 }
 
+#[test]
+fn replicate_overflow_for_a_revenue_submission() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(make_default_full_project());
+		let project_id = ProjectsInfo::<Test>::iter_keys().next().unwrap();
+
+		let revenue_id = get_revenue_id(project_id, 1);
+		let job_eligible_id = get_job_eligible_id(project_id, make_field_name("Job Eligible Test"));
+
+		let revenue_transaction_data =
+			make_revenue_transaction(Some(job_eligible_id), Some(10000), CUDAction::Create, None);
+
+		assert_ok!(FundAdmin::submit_revenue(
+			RuntimeOrigin::signed(2),
+			project_id,
+			revenue_id,
+			Some(revenue_transaction_data),
+			false,
+		));
+
+		let second_revenue_transaction_data = make_revenue_transaction(
+			Some(job_eligible_id),
+			Some(18446744073709551615),
+			CUDAction::Create,
+			None,
+		);
+
+		assert_noop!(
+			FundAdmin::submit_revenue(
+				RuntimeOrigin::signed(2),
+				project_id,
+				revenue_id,
+				Some(second_revenue_transaction_data),
+				true,
+			),
+			Error::<Test>::ArithmeticOverflow
+		);
+	});
+}
+
 #[test]
 fn revenues_an_administrator_rejects_a_given_revenue_works() {
 	new_test_ext().execute_with(|| {