audit fixes #9
audit fixes #9
Conversation
|
Contract comparison - from 2fd5e11 to e6fef64
|
| @@ -43,9 +42,6 @@ pub trait LiquidStaking<ContractReader>: | |||
| #[init] | |||
| fn init(&self) { | |||
| self.state().set(State::Inactive); | |||
| self.max_delegation_addresses() | |||
| .set(MAX_DELEGATION_ADDRESSES); | |||
|
|
|||
| let current_epoch = self.blockchain().get_block_epoch(); | |||
| let claim_status = ClaimStatus { | |||
| status: ClaimStatusType::Insufficient, | |||
There was a problem hiding this comment.
last_claim_block should have been self.blockchain().get_block_nonce(), right?
liquid-staking/src/delegation.rs
Outdated
| @@ -285,6 +280,20 @@ pub trait DelegationModule: | |||
| .get_sc_balance(&EgldOrEsdtTokenIdentifier::egld(), 0) | |||
| - current_total_withdrawn_egld; | |||
| } | |||
|
|
|||
| let mut last_node = current_claim_status.current_node; | |||
| let mut delegation_addresses: ManagedVec<Self::Api, ManagedAddress> = ManagedVec::new(); | |||
There was a problem hiding this comment.
The approach is good, but I don't think it fully resolves the issue. Right now, we initialize a new list with each new claimRewards, which is taken from the delegation_addresses_mapper storage, which may change in between claimRewards operations, especially if they're not emitted consecutively.
What I would suggest is to keep this implementation, but instead of creating the list each time, to have a new vec variable in the ClaimStatus struct and load the list of addresses in the struct directly. From there, we work only on the mutable struct. Then, the next time we try to claimRewards, we both save some gas as we don't create the list again, as well as knowing for sure the list was not updated since our last claim.
There was a problem hiding this comment.
changed to a mapper. This should iterate over and pop addresser from after claiming, as it was suggested in the audit
liquid-staking/src/lib.rs
Outdated
| }); | ||
|
|
||
| total_unstake_amount += unstake_amount; | ||
| providers_unbond_from.push(unstake_token_attributes); |
There was a problem hiding this comment.
What's the use of this variable?
There was a problem hiding this comment.
not sure which one you refer to.. anyway the logic was changed
| #[call_result] result: ManagedAsyncCallResult<()>, | ||
| ) { | ||
| match result { | ||
| ManagedAsyncCallResult::Ok(()) => { | ||
| let withdraw_amount = self.call_value().egld_value().clone_value(); | ||
| let mut storage_cache = StorageCache::new(self); | ||
| let delegation_contract_mapper = | ||
| self.delegation_contract_data(&delegation_contract); | ||
| self.delegation_contract_data(&unstake_token_attributes.delegation_contract); | ||
| if withdraw_amount > 0u64 { |
There was a problem hiding this comment.
It is important to keep in mind that the withdraw process happens at the SC's level, and not for a specific user. I think we should only update the storage (total_withdrawn_egld and total_unbonded_from_ls_contract) inside the if statement withdraw_amount > 0u64. In other words, we should update unbond_from_provider and burn the tokens, regardless the withdraw amount (as the tokens may have been previously withdrawn by other user).
Moreover, maybe we would be more safe if we cover the case of an error as well (by sending back the tokens to the user if the withdraw callback receives an error).
liquid-staking/src/lib.rs
Outdated
| let delegation_contract_data = delegation_contract_mapper.get(); | ||
|
|
||
| if delegation_contract_data.total_unbonded_from_ls_contract < unstake_amount { | ||
| continue; |
There was a problem hiding this comment.
We should really not get to this point, as all unbonds are correlated with an actual user. I would keep the check, but instead panic the SC if it gets to this point.
liquid-staking/src/lib.rs
Outdated
| } | ||
|
|
||
| #[payable("*")] | ||
| #[endpoint(withdrawAll)] |
There was a problem hiding this comment.
Not sure of the naming, as we only withdraw from a single provider, not from "all".
There was a problem hiding this comment.
Jacob's idea. I think he's refering to the funds.. withdraw all funds from the specific provider
framework upgrade to 0.50.1
sc upgrade to framework v0.50.1
Coverage SummaryTotals
FilesExpand
|
fixes based on audit:
https://docs.google.com/document/d/1Tye-zOG6z6vAI_bajvU5iHjlMzel3K8uXZK7sq-adYk/edit