Skip to content

Commit

Permalink
fix overflow in balance_to_point conversion (#2706)
Browse files Browse the repository at this point in the history
Closes #416

As I mentioned in the above issue `balance_to_points` conversion
currently overflows the `Balance` types even before computing the actual
points for the given tokens. This fixes that,
  • Loading branch information
SCJangra authored Dec 22, 2023
1 parent 0ce506e commit 4c0e0e0
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 3 deletions.
9 changes: 6 additions & 3 deletions substrate/frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2955,9 +2955,12 @@ impl<T: Config> Pallet<T> {
},
(false, false) => {
// Equivalent to (current_points / current_balance) * new_funds
balance(u256(current_points).saturating_mul(u256(new_funds)))
// We check for zero above
.div(current_balance)
balance(
u256(current_points)
.saturating_mul(u256(new_funds))
// We check for zero above
.div(u256(current_balance)),
)
},
}
}
Expand Down
22 changes: 22 additions & 0 deletions substrate/frame/nomination-pools/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,28 @@ mod bonded_pool {
assert_ok!(pool.ok_to_join());
});
}

#[test]
fn points_and_balance_conversions_are_safe() {
ExtBuilder::default().build_and_execute(|| {
let bonded_pool = BondedPool::<Runtime> {
id: 123123,
inner: BondedPoolInner {
commission: Commission::default(),
member_counter: 1,
points: u128::MAX,
roles: DEFAULT_ROLES,
state: PoolState::Open,
},
};
StakingMock::set_bonded_balance(bonded_pool.bonded_account(), u128::MAX);

// Max out the points and balance of the pool and make sure the conversion works as
// expected and does not overflow.
assert_eq!(bonded_pool.balance_to_point(u128::MAX), u128::MAX);
assert_eq!(bonded_pool.points_to_balance(u128::MAX), u128::MAX);
})
}
}

mod reward_pool {
Expand Down

0 comments on commit 4c0e0e0

Please sign in to comment.