Merge pull request #996 from multiversx/on-behalf-feature-audit-fixes

onBehalf audit fixes + unit tests

common/modules/original_owner_helper/src/lib.rs

@@ -6,32 +6,40 @@ use common_structs::{FarmToken, PaymentsVec};
 
 #[multiversx_sc::module]
 pub trait OriginalOwnerHelperModule {
-    fn check_and_return_original_owner<T: FarmToken<Self::Api> + TopDecode>(
+    fn get_claim_original_owner<T: FarmToken<Self::Api> + TopDecode>(
         &self,
-        payments: &PaymentsVec<Self::Api>,
         farm_token_mapper: &NonFungibleTokenMapper,
     ) -> ManagedAddress {
-        let mut original_owner = ManagedAddress::zero();
-        for payment in payments.iter() {
+        let payments = self.call_value().all_esdt_transfers();
+        let farm_token_id = farm_token_mapper.get_token_id();
+
+        let mut opt_original_owner = None;
+        for payment in payments.into_iter() {
+            require!(
+                payment.token_identifier == farm_token_id,
+                "Invalid payment token"
+            );
+
             let attributes: T = farm_token_mapper.get_token_attributes(payment.token_nonce);
             let payment_original_owner = attributes.get_original_owner();
 
-            if original_owner.is_zero() {
-                original_owner = payment_original_owner;
-            } else {
-                require!(
-                    original_owner == payment_original_owner,
-                    "All position must have the same original owner"
-                );
+            match opt_original_owner {
+                Some(ref original_owner) => {
+                    require!(
+                        *original_owner == payment_original_owner,
+                        "Original owner is not the same for all payments"
+                    );
+                }
+                None => opt_original_owner = Some(payment_original_owner),
             }
         }
 
         require!(
-            !original_owner.is_zero(),
+            opt_original_owner.is_some(),
             "Original owner could not be identified"
         );
 
-        original_owner
+        unsafe { opt_original_owner.unwrap_unchecked() }
     }
 
     fn check_additional_payments_original_owner<T: FarmToken<Self::Api> + TopDecode>(

common/modules/permissions_hub_module/src/permissions_hub_module.rs

@@ -12,7 +12,10 @@ pub trait PermissionsHubModule {
             .is_whitelisted(user, authorized_address)
             .execute_on_dest_context();
 
-        require!(is_whitelisted, "Caller is not whitelisted by the user");
+        require!(
+            is_whitelisted,
+            "Caller is not whitelisted by the user or is blacklisted"
+        );
     }
 
     #[only_owner]

dex/farm-with-locked-rewards/src/external_interaction.rs

@@ -55,6 +55,8 @@ pub trait ExternalInteractionsModule:
             &farm_token_mapper,
         );
 
+        self.migrate_old_farm_positions(&user);
+
         let boosted_rewards = self.claim_only_boosted_payment(&user);
         let new_farm_token = self.enter_farm::<NoMintWrapper<Self>>(user.clone());
         self.send_payment_non_zero(&caller, &new_farm_token);
@@ -79,32 +81,41 @@ pub trait ExternalInteractionsModule:
     #[payable("*")]
     #[endpoint(claimRewardsOnBehalf)]
     fn claim_rewards_on_behalf(&self) -> ClaimRewardsResultType<Self::Api> {
-        let payments = self.get_non_empty_payments();
         let farm_token_mapper = self.farm_token();
         let caller = self.blockchain().get_caller();
-        let user = self.check_and_return_original_owner::<FarmTokenAttributes<Self::Api>>(
-            &payments,
-            &farm_token_mapper,
-        );
+        let user =
+            self.get_claim_original_owner::<FarmTokenAttributes<Self::Api>>(&farm_token_mapper);
         self.require_user_whitelisted(&user, &caller);
 
+        self.migrate_old_farm_positions(&user);
+
         let claim_rewards_result = self.claim_rewards::<NoMintWrapper<Self>>(user.clone());
 
         self.send_payment_non_zero(&caller, &claim_rewards_result.new_farm_token);
 
         let rewards_payment = claim_rewards_result.rewards;
-        let locked_rewards_payment = if rewards_payment.amount == 0 {
-            let locked_token_id = self.get_locked_token_id();
-            EsdtTokenPayment::new(locked_token_id, 0, rewards_payment.amount)
-        } else {
-            self.lock_virtual(
-                rewards_payment.token_identifier,
-                rewards_payment.amount,
-                user.clone(),
-                user,
-            )
-        };
+        let locked_rewards_payment = self.send_to_lock_contract_non_zero(
+            rewards_payment.token_identifier,
+            rewards_payment.amount,
+            user.clone(),
+            user,
+        );
 
         (claim_rewards_result.new_farm_token, locked_rewards_payment).into()
     }
+
+    fn send_to_lock_contract_non_zero(
+        &self,
+        token_id: TokenIdentifier,
+        amount: BigUint,
+        destination_address: ManagedAddress,
+        energy_address: ManagedAddress,
+    ) -> EsdtTokenPayment {
+        if amount == 0 {
+            let locked_token_id = self.get_locked_token_id();
+            return EsdtTokenPayment::new(locked_token_id, 0, amount);
+        }
+
+        self.lock_virtual(token_id, amount, destination_address, energy_address)
+    }
 }

dex/farm-with-locked-rewards/src/lib.rs

@@ -301,21 +301,6 @@ pub trait Farm:
             &storage_cache,
         )
     }
-
-    fn send_to_lock_contract_non_zero(
-        &self,
-        token_id: TokenIdentifier,
-        amount: BigUint,
-        destination_address: ManagedAddress,
-        energy_address: ManagedAddress,
-    ) -> EsdtTokenPayment {
-        if amount == 0 {
-            let locked_token_id = self.get_locked_token_id();
-            return EsdtTokenPayment::new(locked_token_id, 0, amount);
-        }
-
-        self.lock_virtual(token_id, amount, destination_address, energy_address)
-    }
 }
 
 pub struct NoMintWrapper<T: BaseFunctionsModule + farm_boosted_yields::FarmBoostedYieldsModule> {

dex/farm/src/external_interaction.rs

@@ -45,6 +45,8 @@ pub trait ExternalInteractionsModule:
         let caller = self.blockchain().get_caller();
         self.require_user_whitelisted(&user, &caller);
 
+        self.migrate_old_farm_positions(&user);
+
         let payments = self.get_non_empty_payments();
         let farm_token_mapper = self.farm_token();
         self.check_additional_payments_original_owner::<FarmTokenAttributes<Self::Api>>(
@@ -70,16 +72,15 @@ pub trait ExternalInteractionsModule:
     #[payable("*")]
     #[endpoint(claimRewardsOnBehalf)]
     fn claim_rewards_on_behalf(&self) -> ClaimRewardsResultType<Self::Api> {
-        let payments = self.get_non_empty_payments();
         let farm_token_mapper = self.farm_token();
 
         let caller = self.blockchain().get_caller();
-        let user = self.check_and_return_original_owner::<FarmTokenAttributes<Self::Api>>(
-            &payments,
-            &farm_token_mapper,
-        );
+        let user =
+            self.get_claim_original_owner::<FarmTokenAttributes<Self::Api>>(&farm_token_mapper);
         self.require_user_whitelisted(&user, &caller);
 
+        self.migrate_old_farm_positions(&user);
+
         let claim_rewards_result = self.claim_rewards::<Wrapper<Self>>(user.clone());
 
         self.send_payment_non_zero(&caller, &claim_rewards_result.new_farm_token);

dex/farm/tests/external_interaction_test.rs

@@ -194,7 +194,7 @@ fn test_enter_and_claim_farm_on_behalf_not_whitelisted_error() {
                 sc.enter_farm_on_behalf(managed_address!(&external_user));
             },
         )
-        .assert_error(4, "Caller is not whitelisted by the user");
+        .assert_error(4, "Caller is not whitelisted by the user or is blacklisted");
 
     let farm_token_amount = 100_000_000;
     farm_setup.whitelist_address_on_behalf(&external_user, &authorized_address);
@@ -214,7 +214,7 @@ fn test_enter_and_claim_farm_on_behalf_not_whitelisted_error() {
                 sc.claim_rewards_on_behalf();
             },
         )
-        .assert_error(4, "Caller is not whitelisted by the user");
+        .assert_error(4, "Caller is not whitelisted by the user or is blacklisted");
 }
 
 #[test]
@@ -305,5 +305,149 @@ fn test_wrong_original_owner_on_behalf_validation() {
                 sc.claim_rewards_on_behalf();
             },
         )
-        .assert_error(4, "All position must have the same original owner");
+        .assert_error(4, "Original owner is not the same for all payments");
+
+    // Check enter on behalf with blacklisted address
+    let blacklisted_address = farm_setup.b_mock.create_user_account(&rust_biguint!(0));
+    farm_setup.whitelist_address_on_behalf(&external_user1, &blacklisted_address);
+    farm_setup.blacklist_address_on_behalf(&blacklisted_address);
+
+    farm_setup.b_mock.set_esdt_balance(
+        &blacklisted_address,
+        FARMING_TOKEN_ID,
+        &rust_biguint!(farm_token_amount),
+    );
+
+    farm_setup
+        .b_mock
+        .execute_esdt_transfer(
+            &blacklisted_address,
+            &farm_setup.farm_wrapper,
+            FARMING_TOKEN_ID,
+            0,
+            &rust_biguint!(farm_token_amount),
+            |sc| {
+                sc.enter_farm_on_behalf(managed_address!(&external_user1));
+            },
+        )
+        .assert_error(4, "Caller is not whitelisted by the user or is blacklisted");
+}
+
+#[test]
+fn test_multiple_position_claim_on_behalf_average_rps_computation() {
+    DebugApi::dummy();
+
+    let mut farm_setup = MultiUserFarmSetup::new(
+        farm::contract_obj,
+        energy_factory_mock::contract_obj,
+        energy_update::contract_obj,
+        permissions_hub::contract_obj,
+    );
+
+    farm_setup.set_boosted_yields_rewards_percentage(BOOSTED_YIELDS_PERCENTAGE);
+    farm_setup.set_boosted_yields_factors();
+    let mut block_nonce = 0u64;
+    farm_setup.b_mock.set_block_nonce(block_nonce);
+
+    // new external user
+    let external_user = farm_setup.b_mock.create_user_account(&rust_biguint!(0));
+    farm_setup.set_user_energy(&external_user, 1_000, 1, 1);
+
+    // authorized address
+    let farm_token_amount = 100_000_000;
+    let authorized_address = farm_setup.first_user.clone();
+
+    farm_setup.whitelist_address_on_behalf(&external_user, &authorized_address);
+
+    farm_setup.check_farm_token_supply(0);
+    farm_setup.enter_farm_on_behalf(
+        &authorized_address,
+        &external_user,
+        farm_token_amount / 2,
+        0,
+        0,
+    );
+    farm_setup.check_farm_token_supply(farm_token_amount / 2);
+
+    farm_setup.enter_farm_on_behalf(
+        &authorized_address,
+        &external_user,
+        farm_token_amount / 2,
+        0,
+        0,
+    );
+    farm_setup.check_farm_token_supply(farm_token_amount);
+
+    let block_nonce_diff = 10u64;
+    block_nonce += block_nonce_diff;
+    farm_setup.b_mock.set_block_nonce(block_nonce);
+
+    // 1000 rewards per block
+    let total_rewards = PER_BLOCK_REWARD_AMOUNT * block_nonce_diff;
+    let base_rewards =
+        total_rewards * (MAX_PERCENTAGE - BOOSTED_YIELDS_PERCENTAGE) / MAX_PERCENTAGE;
+
+    // Claim first base rewards
+    farm_setup
+        .b_mock
+        .check_esdt_balance(&external_user, REWARD_TOKEN_ID, &rust_biguint!(0));
+
+    let mut payments = Vec::new();
+    payments.push(TxTokenTransfer {
+        token_identifier: FARM_TOKEN_ID.to_vec(),
+        nonce: 1,
+        value: rust_biguint!(farm_token_amount / 2),
+    });
+    payments.push(TxTokenTransfer {
+        token_identifier: FARM_TOKEN_ID.to_vec(),
+        nonce: 2,
+        value: rust_biguint!(farm_token_amount / 2),
+    });
+
+    // First claim, without any position merged
+    // Should give rewards for the first payment only and compute average RPS from both positions
+    farm_setup
+        .b_mock
+        .execute_esdt_multi_transfer(
+            &authorized_address,
+            &farm_setup.farm_wrapper,
+            &payments,
+            |sc| {
+                sc.claim_rewards_on_behalf();
+            },
+        )
+        .assert_ok();
+
+    farm_setup.b_mock.check_esdt_balance(
+        &external_user,
+        REWARD_TOKEN_ID,
+        &rust_biguint!(base_rewards / 2),
+    );
+
+    payments = Vec::new();
+    payments.push(TxTokenTransfer {
+        token_identifier: FARM_TOKEN_ID.to_vec(),
+        nonce: 3,
+        value: rust_biguint!(farm_token_amount),
+    });
+
+    // Second claim, with both position merged, in the same block
+    // Should give the rest of the rewards (for the same positions)
+    farm_setup
+        .b_mock
+        .execute_esdt_multi_transfer(
+            &authorized_address,
+            &farm_setup.farm_wrapper,
+            &payments,
+            |sc| {
+                sc.claim_rewards_on_behalf();
+            },
+        )
+        .assert_ok();
+
+    farm_setup.b_mock.check_esdt_balance(
+        &external_user,
+        REWARD_TOKEN_ID,
+        &rust_biguint!(base_rewards),
+    );
 }

dex/farm/tests/farm_setup/multi_user_farm_setup.rs

@@ -685,6 +685,19 @@ where
             .assert_ok();
     }
 
+    pub fn blacklist_address_on_behalf(&mut self, address_to_blacklist: &Address) {
+        self.b_mock
+            .execute_tx(
+                &self.owner,
+                &self.permissions_hub_wrapper,
+                &rust_biguint!(0),
+                |sc| {
+                    sc.blacklist(managed_address!(address_to_blacklist));
+                },
+            )
+            .assert_ok();
+    }
+
     pub fn enter_farm_on_behalf(
         &mut self,
         caller: &Address,