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 2 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
22 changes: 9 additions & 13 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 @@ -54,21 +54,17 @@ 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;
});
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);
}

#[view(getFarmingTokenId)]
Expand Down Expand Up @@ -101,7 +97,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
9 changes: 7 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,9 @@ 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
self.try_set_farm_position_migration_nonce();
}

#[payable("*")]
Expand Down Expand Up @@ -155,11 +158,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
60 changes: 55 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,68 @@ 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 try_set_farm_position_migration_nonce(&self) {
if !self.farm_position_migration_nonce().is_empty() {
return;
}

let farm_token_mapper = self.farm_token();

let attributes = FarmTokenAttributes {
reward_per_share: BigUint::zero(),
entering_epoch: 0,
compounded_reward: BigUint::zero(),
current_farm_amount: BigUint::zero(),
original_owner: self.blockchain().get_sc_address(),
};

let migration_farm_token_nonce = if farm_token_mapper.get_token_state().is_set() {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to create new token. Use VM endpoint GetCurrentESDTNFTNonce / implement it to Rust Framework if it does not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

let migration_farm_token =
farm_token_mapper.nft_create(BigUint::from(1u64), &attributes);
farm_token_mapper.nft_burn(
migration_farm_token.token_nonce,
&migration_farm_token.amount,
);
migration_farm_token.token_nonce
} else {
DEFAULT_FARM_POSITION_MIGRATION_NONCE
};

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

fn end_produce_rewards<FC: FarmContract<FarmSc = Self>>(&self) {
Expand Down
8 changes: 4 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,7 @@ 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);
self.try_set_farm_position_migration_nonce();
}

#[payable("*")]
Expand Down Expand Up @@ -159,10 +157,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
35 changes: 35 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 @@ -25,6 +25,8 @@ pub mod token_attributes;
pub mod unbond_farm;
pub mod unstake_farm;

pub const DEFAULT_FARM_POSITION_MIGRATION_NONCE: u64 = 1;

#[multiversx_sc::contract]
pub trait FarmStaking:
custom_rewards::CustomRewardsModule
Expand Down Expand Up @@ -88,6 +90,9 @@ pub trait FarmStaking:
"Invalid min unbond epochs"
);
self.min_unbond_epochs().set_if_empty(min_unbond_epochs);

// Farm position migration code
self.try_set_farm_position_migration_nonce();
}

#[payable("*")]
Expand Down Expand Up @@ -144,4 +149,34 @@ pub trait FarmStaking:
"May only call this function through VM query"
);
}

fn try_set_farm_position_migration_nonce(&self) {
if !self.farm_position_migration_nonce().is_empty() {
return;
}

let farm_token_mapper = self.farm_token();

let attributes = StakingFarmTokenAttributes {
reward_per_share: BigUint::zero(),
compounded_reward: BigUint::zero(),
current_farm_amount: BigUint::zero(),
original_owner: self.blockchain().get_sc_address(),
};

let migration_farm_token_nonce = if farm_token_mapper.get_token_state().is_set() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above - use GetCurrentESDTNFTNonce. Why do you have duplicated code ? Base function should resolv this - as code is exactly the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue for the code duplication was farm_token. It has different attributes for farm and farm-staking.
I update the code: moved this to common module and provide the token_mapper as a parameter.

let migration_farm_token =
farm_token_mapper.nft_create(BigUint::from(1u64), &attributes);
farm_token_mapper.nft_burn(
migration_farm_token.token_nonce,
&migration_farm_token.amount,
);
migration_farm_token.token_nonce
} else {
DEFAULT_FARM_POSITION_MIGRATION_NONCE
};

self.farm_position_migration_nonce()
.set(migration_farm_token_nonce);
}
}
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