Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove BONDED_AMOUNT and additional query/execute functionality #202

Merged
merged 25 commits into from
Nov 29, 2024

Conversation

faust403
Copy link
Member

@faust403 faust403 commented Nov 19, 2024

PR includes some of proposed audit fixes:

  • Remove bond_limit & BONDED_AMOUNT
  • Insufficient validations in bond provider
  • Missing validations in configuration
    Missing input deduplication
  • Inconsistent naming of function and return value
  • Unnecessary reply handlers
    Unfinished development
  • Redundant code
    Usage of panic for handling errors is discouraged

@faust403 faust403 changed the base branch from dev to feat/audit-fixes-oak-2024-11 November 20, 2024 14:43
Comment on lines 372 to 381
let bond_provider_balances = deps
.querier
.query_all_balances(bond_provider_address.clone())?;
let bond_provider_balances_except_untrn = bond_provider_balances
.into_iter()
.filter(|coin| coin.denom != *UNTRN_DENOM.to_string())
.collect::<Vec<Coin>>();
if !bond_provider_balances_except_untrn.is_empty() {
return Err(ContractError::BondProviderBalanceNotEmpty {});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add query interface to the bond provider and let him answer can it be removed or not. Because there is internal state in the provider and it relates not only to bank balance but on other operations as well. So the real way to remove provider on working protocol is to pause bonding, wait for all balances and internal states becomes normal and only after that we can safely remove bond provider

Comment on lines +117 to +121
let all_balances = deps.querier.query_all_balances(env.contract.address)?;
let all_balances_except_untrn = all_balances
.into_iter()
.filter(|coin| coin.denom != *LOCAL_DENOM.to_string())
.collect::<Vec<Coin>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not enough, we also need to be sure there is no any PENDING_LSM_SHARES and LSM_SHARES_TO_REDEEM, and only after that we are safe to remove provider (during one block)

Comment on lines +108 to +112
let all_balances = deps.querier.query_all_balances(env.contract.address)?;
let all_balances_except_untrn = all_balances
.into_iter()
.filter(|coin| coin.denom != *LOCAL_DENOM.to_string())
.collect::<Vec<Coin>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we also need to check for empty NON_STAKED_BALANCE

@oldremez oldremez merged commit 31f7b90 into feat/audit-fixes-oak-2024-11 Nov 29, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants