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.

Sign up for GitHub

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

audit fixes (2) #775

Merged
merged 5 commits into from
Sep 26, 2023
Merged

audit fixes (2) #775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bbff637
audit fixes (2)
psorinionut Sep 20, 2023
4303c0c
default migration nonce fix
psorinionut Sep 20, 2023
767f452
use GetCurrentESDTNFTNonce VM endpoint
psorinionut Sep 21, 2023
8100d57
try_set_farm_position_migration_nonce: code dup
CostinCarabas Sep 21, 2023
aba2e4e
fix farm_position_migration_nonce set function
psorinionut Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 35 additions & 16 deletions common/modules/farm/config/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use common_structs::Nonce;
use pausable::State;

pub const DEFAULT_NFT_DEPOSIT_MAX_LEN: usize = 10;
pub const DEFAULT_FARM_POSITION_MIGRATION_NONCE: u64 = 1;

#[derive(
ManagedVecItem,
Expand Down Expand Up @@ -35,6 +36,15 @@ impl<M: ManagedTypeApi> Default for UserTotalFarmPosition<M> {

#[multiversx_sc::module]
pub trait ConfigModule: pausable::PausableModule + permissions_module::PermissionsModule {
#[endpoint(allowExternalClaimBoostedRewards)]
fn allow_external_claim_boosted_rewards(&self, allow_external_claim: bool) {
let caller = self.blockchain().get_caller();
let mut user_total_farm_position = self.get_user_total_farm_position(&caller);
user_total_farm_position.allow_external_claim_boosted_rewards = allow_external_claim;
self.user_total_farm_position(&caller)
.set(user_total_farm_position);
}

#[inline]
fn is_active(&self) -> bool {
let state = self.state().get();
Expand All @@ -54,21 +64,30 @@ pub trait ConfigModule: pausable::PausableModule + permissions_module::Permissio
}

fn is_old_farm_position(&self, token_nonce: Nonce) -> bool {
let farm_position_migration_block_nonce = self.farm_position_migration_block_nonce().get();
token_nonce > 0 && token_nonce < farm_position_migration_block_nonce
let farm_position_migration_nonce = self.farm_position_migration_nonce().get();
token_nonce > 0 && token_nonce < farm_position_migration_nonce
}

#[endpoint(allowExternalClaimBoostedRewards)]
fn allow_external_claim_boosted_rewards(&self, allow_external_claim: bool) {
let caller = self.blockchain().get_caller();
let user_total_farm_position_mapper = self.user_total_farm_position(&caller);
require!(
!user_total_farm_position_mapper.is_empty(),
"User must have a farm position"
);
user_total_farm_position_mapper.update(|user_total_farm_position| {
user_total_farm_position.allow_external_claim_boosted_rewards = allow_external_claim;
});
fn try_set_farm_position_migration_nonce(
&self,
farm_token_mapper: NonFungibleTokenMapper<Self::Api>,
) {
if !self.farm_position_migration_nonce().is_empty() {
return;
}

let migration_farm_token_nonce = if farm_token_mapper.get_token_state().is_set() {
let token_identifier = farm_token_mapper.get_token_id_ref();
let current_nonce = self
.blockchain()
.get_current_esdt_nft_nonce(&self.blockchain().get_sc_address(), token_identifier);
current_nonce + DEFAULT_FARM_POSITION_MIGRATION_NONCE
} else {
DEFAULT_FARM_POSITION_MIGRATION_NONCE
};

self.farm_position_migration_nonce()
.set(migration_farm_token_nonce);
}

#[view(getFarmingTokenId)]
Expand Down Expand Up @@ -101,7 +120,7 @@ pub trait ConfigModule: pausable::PausableModule + permissions_module::Permissio
user: &ManagedAddress,
) -> SingleValueMapper<UserTotalFarmPosition<Self::Api>>;

#[view(getFarmPositionMigrationBlockNonce)]
#[storage_mapper("farm_position_migration_block_nonce")]
fn farm_position_migration_block_nonce(&self) -> SingleValueMapper<Nonce>;
#[view(getFarmPositionMigrationNonce)]
#[storage_mapper("farm_position_migration_nonce")]
fn farm_position_migration_nonce(&self) -> SingleValueMapper<Nonce>;
}
4 changes: 2 additions & 2 deletions common/modules/farm/farm_base_impl/src/base_traits_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,12 @@ pub trait FarmContract {
) {
let farm_token_mapper = sc.farm_token();
for farm_position in farm_positions {
farm_token_mapper.require_same_token(&farm_position.token_identifier);

if sc.is_old_farm_position(farm_position.token_nonce) {
continue;
}

farm_token_mapper.require_same_token(&farm_position.token_identifier);

let token_attributes: FarmTokenAttributes<<Self::FarmSc as ContractBase>::Api> =
farm_token_mapper.get_token_attributes(farm_position.token_nonce);

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we optimize here the storage access for user_farm_position? After the for, settle with increase/decrease of user_farm_position.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract the increase/decrease of the user_farm_position outside of the for, to improve the gas consumption?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it in the new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it can be used only for increase position, as decrease position function needs the entire looped payment to check the attributes and decrease for the original owner. I'll see if there is any improvement if we send the original_owner as a parameter, instead of the payment.

Expand Down
10 changes: 8 additions & 2 deletions dex/farm-with-locked-rewards/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ pub trait Farm:

let current_epoch = self.blockchain().get_block_epoch();
self.first_week_start_epoch().set_if_empty(current_epoch);

// Farm position migration code
let farm_token_mapper = self.farm_token();
self.try_set_farm_position_migration_nonce(farm_token_mapper);
}

#[payable("*")]
Expand Down Expand Up @@ -155,11 +159,13 @@ pub trait Farm:

let payment = self.call_value().single_esdt();

self.migrate_old_farm_positions(&orig_caller);
let migrated_amount = self.migrate_old_farm_positions(&orig_caller);

let exit_farm_result = self.exit_farm::<NoMintWrapper<Self>>(orig_caller.clone(), payment);
let rewards = exit_farm_result.rewards;

self.decrease_old_farm_positions(migrated_amount, &orig_caller);

let rewards = exit_farm_result.rewards;
self.send_payment_non_zero(&caller, &exit_farm_result.farming_tokens);

let locked_rewards_payment = self.send_to_lock_contract_non_zero(
Expand Down
2 changes: 1 addition & 1 deletion dex/farm-with-locked-rewards/wasm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ multiversx_sc_wasm_adapter::endpoints! {
getLastRewardBlockNonce => last_reward_block_nonce
getDivisionSafetyConstant => division_safety_constant
getUserTotalFarmPosition => user_total_farm_position
getFarmPositionMigrationBlockNonce => farm_position_migration_block_nonce
getFarmPositionMigrationNonce => farm_position_migration_nonce
setLockingScAddress => set_locking_sc_address
setLockEpochs => set_lock_epochs
getLockingScAddress => locking_sc_address
Expand Down
29 changes: 24 additions & 5 deletions dex/farm/src/base_functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ pub type DoubleMultiPayment<M> = MultiValue2<EsdtTokenPayment<M>, EsdtTokenPayme
pub type ClaimRewardsResultType<M> = DoubleMultiPayment<M>;
pub type ExitFarmResultType<M> = DoubleMultiPayment<M>;

pub const DEFAULT_FARM_POSITION_MIGRATION_NONCE: u64 = 1;

pub struct ClaimRewardsResultWrapper<M: ManagedTypeApi> {
pub new_farm_token: EsdtTokenPayment<M>,
pub rewards: EsdtTokenPayment<M>,
Expand Down Expand Up @@ -204,20 +206,37 @@ pub trait BaseFunctionsModule:
reward
}

fn migrate_old_farm_positions(&self, caller: &ManagedAddress) {
fn migrate_old_farm_positions(&self, caller: &ManagedAddress) -> BigUint {
let payments = self.get_non_empty_payments();
let farm_token_mapper = self.farm_token();
let farm_token_id = farm_token_mapper.get_token_id();
let mut migrated_amount = BigUint::zero();
for farm_position in &payments {
if farm_position.token_identifier == farm_token_id
&& self.is_old_farm_position(farm_position.token_nonce)
{
let mut user_total_farm_position = self.get_user_total_farm_position(caller);
user_total_farm_position.total_farm_position += farm_position.amount;
self.user_total_farm_position(caller)
.set(user_total_farm_position);
migrated_amount += farm_position.amount;
}
}

if migrated_amount > 0 {
let mut user_total_farm_position = self.get_user_total_farm_position(caller);
user_total_farm_position.total_farm_position += &migrated_amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

A gas improvement here could be an update instead of a get + set. (see the decrease_old_farm_positions function below).

self.get_user_total_farm_position() also verifies if position is empty, so a new function could do that (verification + update)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this optimizes the gas cost as much, as we still need to check if the storage is empty before the update function. And also, this would mean another function, which would increase the contract size (even if by a small margin).

self.user_total_farm_position(caller)
.set(user_total_farm_position);
}

migrated_amount
}

fn decrease_old_farm_positions(&self, migrated_amount: BigUint, caller: &ManagedAddress) {
if migrated_amount == BigUint::zero() {
return;
}
self.user_total_farm_position(caller)
.update(|user_total_farm_position| {
user_total_farm_position.total_farm_position -= migrated_amount;
});
}

fn end_produce_rewards<FC: FarmContract<FarmSc = Self>>(&self) {
Expand Down
9 changes: 5 additions & 4 deletions dex/farm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,8 @@ pub trait Farm:
self.first_week_start_epoch().set_if_empty(current_epoch);

// Farm position migration code
let block_nonce = self.blockchain().get_block_nonce();
self.farm_position_migration_block_nonce()
.set_if_empty(block_nonce);
let farm_token_mapper = self.farm_token();
self.try_set_farm_position_migration_nonce(farm_token_mapper);
}

#[payable("*")]
Expand Down Expand Up @@ -159,10 +158,12 @@ pub trait Farm:

let payment = self.call_value().single_esdt();

self.migrate_old_farm_positions(&orig_caller);
let migrated_amount = self.migrate_old_farm_positions(&orig_caller);

let exit_farm_result = self.exit_farm::<Wrapper<Self>>(orig_caller.clone(), payment);

self.decrease_old_farm_positions(migrated_amount, &orig_caller);

self.send_payment_non_zero(&caller, &exit_farm_result.farming_tokens);
self.send_payment_non_zero(&caller, &exit_farm_result.rewards);

Expand Down
2 changes: 1 addition & 1 deletion dex/farm/wasm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ multiversx_sc_wasm_adapter::endpoints! {
getLastRewardBlockNonce => last_reward_block_nonce
getDivisionSafetyConstant => division_safety_constant
getUserTotalFarmPosition => user_total_farm_position
getFarmPositionMigrationBlockNonce => farm_position_migration_block_nonce
getFarmPositionMigrationNonce => farm_position_migration_nonce
registerFarmToken => register_farm_token
getFarmTokenId => farm_token
getFarmTokenSupply => farm_token_supply
Expand Down
27 changes: 22 additions & 5 deletions farm-staking/farm-staking/src/claim_only_boosted_staking_rewards.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,37 @@ pub trait ClaimOnlyBoostedStakingRewardsModule:
boosted_rewards_payment
}

fn migrate_old_farm_positions(&self, caller: &ManagedAddress) {
fn migrate_old_farm_positions(&self, caller: &ManagedAddress) -> BigUint {
let payments = self.call_value().all_esdt_transfers().clone_value();
let farm_token_mapper = self.farm_token();
let farm_token_id = farm_token_mapper.get_token_id();
let mut migrated_amount = BigUint::zero();
for farm_position in &payments {
if farm_position.token_identifier == farm_token_id
&& self.is_old_farm_position(farm_position.token_nonce)
{
let mut user_total_farm_position = self.get_user_total_farm_position(caller);
user_total_farm_position.total_farm_position += farm_position.amount;
self.user_total_farm_position(caller)
.set(user_total_farm_position);
migrated_amount += farm_position.amount;
}
}

if migrated_amount > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have duplicated code ? Can't you add this into base function ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The farm staking contract does not import the base_functions module as there are some variables that are Farm only specific. Maybe we could rewrite this to be more generic.

Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot do that due to Token Attributes. I think it's difficult to rewrite the code write now. The code changes will be huge and we would have to re-audit the whole solution.

let mut user_total_farm_position = self.get_user_total_farm_position(caller);
user_total_farm_position.total_farm_position += &migrated_amount;
self.user_total_farm_position(caller)
.set(user_total_farm_position);
}

migrated_amount
}

fn decrease_old_farm_positions(&self, migrated_amount: BigUint, caller: &ManagedAddress) {
if migrated_amount == BigUint::zero() {
return;
}
self.user_total_farm_position(caller)
.update(|user_total_farm_position| {
user_total_farm_position.total_farm_position -= migrated_amount;
});
}

// Cannot import the one from farm, as the Wrapper struct has different dependencies
Expand Down
4 changes: 4 additions & 0 deletions farm-staking/farm-staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ pub trait FarmStaking:
"Invalid min unbond epochs"
);
self.min_unbond_epochs().set_if_empty(min_unbond_epochs);

// Farm position migration code
let farm_token_mapper = self.farm_token();
self.try_set_farm_position_migration_nonce(farm_token_mapper);
}

#[payable("*")]
Expand Down
4 changes: 3 additions & 1 deletion farm-staking/farm-staking/src/unstake_farm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,13 @@ pub trait UnstakeFarmModule:
payment: EsdtTokenPayment,
opt_unbond_amount: Option<BigUint>,
) -> ExitFarmWithPartialPosResultType<Self::Api> {
self.migrate_old_farm_positions(&original_caller);
let migrated_amount = self.migrate_old_farm_positions(&original_caller);

let exit_result =
self.exit_farm_base::<FarmStakingWrapper<Self>>(original_caller.clone(), payment);

self.decrease_old_farm_positions(migrated_amount, &original_caller);

let unbond_token_amount =
opt_unbond_amount.unwrap_or(exit_result.farming_token_payment.amount);
let farm_token_id = exit_result.storage_cache.farm_token_id.clone();
Expand Down
2 changes: 1 addition & 1 deletion farm-staking/farm-staking/wasm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ multiversx_sc_wasm_adapter::endpoints! {
getLastRewardBlockNonce => last_reward_block_nonce
getDivisionSafetyConstant => division_safety_constant
getUserTotalFarmPosition => user_total_farm_position
getFarmPositionMigrationBlockNonce => farm_position_migration_block_nonce
getFarmPositionMigrationNonce => farm_position_migration_nonce
registerFarmToken => register_farm_token
getFarmTokenId => farm_token
getFarmTokenSupply => farm_token_supply
Expand Down
Loading