Skip to content

Commit

Permalink
- Added a safe addition helper function, safe_add, to the Fund Admi…
Browse files Browse the repository at this point in the history
…n Pallet to mitigate risks from arithmetic overflow. This function checks if the addition of two numbers results in an overflow and, if so, throws the `ArithmeticOverflow` error. This check has been incorporated into the `do_calculate_drawdown_total_amount` and `do_calculate_revenue_total_amount` functions to ensure safe addition operation and provide user feedback.

- Defined a new `ArithmeticOverflow` error in the Fund Admin Pallet to provide feedback for operations that can potentially cause arithmetic overflow. This helps increase the robustness of our numeric operations.
- Introduced two new test cases `replicate_overflow_for_a_drawdown_submission` and `replicate_overflow_for_a_revenue_submission` in the Fund Admin Pallet test suite to ensure accurate arithmetic overflow detection. These tests validate that the system correctly rejects drawdown and revenue submissions which would result in arithmetic overflow conditions. This expands the coverage of our testing and ensures the safe operation of financial transactions.
  • Loading branch information
didiermis committed Mar 21, 2024
1 parent 69418f7 commit e8244e8
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 7 deletions.
19 changes: 12 additions & 7 deletions pallets/fund-admin/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2008,13 +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);

// Ensure revenue transaction id doesn't exist
ensure!(
!RevenueTransactionsInfo::<T>::contains_key(revenue_transaction_id),
Error::<T>::RevenueTransactionIdAlreadyExists
);
(revenue_id, revenue_amount, job_eligible_id, project_id, timestamp)
.using_encoded(blake2_256);

// Create revenue transaction data
let revenue_transaction_data = RevenueTransactionData {
Expand Down Expand Up @@ -3125,6 +3120,11 @@ impl<T: Config> Pallet<T> {
Ok(())
}

fn safe_add(a: u64, b: u64) -> DispatchResult {
a.checked_add(b).ok_or_else(|| Error::<T>::ArithmeticOverflow)?;
Ok(())
}

fn do_calculate_drawdown_total_amount(
project_id: [u8; 32],
drawdown_id: [u8; 32],
Expand All @@ -3145,6 +3145,8 @@ impl<T: Config> Pallet<T> {
// Get transaction data
let transaction_data = TransactionsInfo::<T>::get(transaction_id)
.ok_or(Error::<T>::TransactionNotFound)?;
// Check arithmetic overflow
Self::safe_add(drawdown_total_amount, transaction_data.amount)?;

// Add transaction amount to drawdown total amount
drawdown_total_amount += transaction_data.amount;
Expand Down Expand Up @@ -3298,6 +3300,9 @@ impl<T: Config> Pallet<T> {
RevenueTransactionsInfo::<T>::get(revenue_transaction_id)
.ok_or(Error::<T>::RevenueTransactionNotFound)?;

// Check arithmetic overflow
Self::safe_add(revenue_total_amount, revenue_transaction_data.amount)?;

// Add revenue transaction amount to revenue total amount
revenue_total_amount += revenue_transaction_data.amount;
}
Expand Down
2 changes: 2 additions & 0 deletions pallets/fund-admin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions pallets/fund-admin/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
84 changes: 84 additions & 0 deletions pallets/fund-admin/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(|| {
Expand Down Expand Up @@ -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(|| {
Expand Down

0 comments on commit e8244e8

Please sign in to comment.