Function with public attribute are not correctly implemented

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x5f3db33b785c1d1ebccaa80da95e44d793c070768c8ae8793fbce289bf77cf68 Severity: medium

Description: Description\ All functions inside farm contract module impl Farm for FarmContract are not declared public as they are declared with #[ink(message)] attribute which is meant to be used strictly with public functions as per Ink! official docs.

Attack Scenario\ Functions declared without #[ink(message)] attribute make them private which cannot be accessed externally making the whole contract useless and inoperable from outside.

Some functions are vital for protocol i.e., claim_rewards, deposit, deposit_all and withdraw which required interaction with user and thus making whole farm contract unable to operate.

Attachments

https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/blob/0a7264d707aea51b559a1bf94448681b59660f6a/farm/contract/lib.rs#L260C3-L421C6

1. Proof of Concept (PoC) File

Every function defined with #[ink(message)] attribute but is not declared public in Farm module.

    impl Farm for FarmContract {
        #[ink(message)]
        fn pool_id(&self) -> AccountId {
            self.pool_id
        }
        #[ink(message)]
        fn total_shares(&self) -> u128 {
            self.total_shares
        }

        #[ink(message)]
        fn shares_of(&self, owner: AccountId) -> u128 {
            self.shares.get(owner).unwrap_or(0)
        }

        #[ink(message)]
        fn reward_tokens(&self) -> Vec<AccountId> {
            self.reward_tokens.clone()
        }

        #[ink(message)]
        fn owner_start_new_farm(
            &mut self,
            start: Timestamp,
            end: Timestamp,
            rewards: Vec<u128>,
        ) -> Result<(), FarmError> {
            ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner);
            self.update()?;
            ensure!(!self.is_active(), FarmError::FarmAlreadyRunning);
            self.farm_reward_rates = self.assert_start_params(start, end, rewards.clone())?;
            self.start = start;
            self.end = end;
            Ok(())
        }

        #[ink(message)]
        fn owner_stop_farm(&mut self) -> Result<(), FarmError> {
            ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner);
            self.update()?;
            self.end = self.env().block_timestamp();
            Ok(())
        }

        #[ink(message)]
        fn owner_withdraw_token(&mut self, token: TokenId) -> Result<(), FarmError> {
            ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner);
            ensure!(!self.is_active(), FarmError::FarmAlreadyRunning);
            // Owner should be able to withdraw every token except the pool token.
            ensure!(self.pool_id != token, FarmError::RewardTokenIsPoolToken);
            self.update()?;
            let mut token_ref: contract_ref!(PSP22) = token.into();
            let total_balance = token_ref.balance_of(self.env().account_id());
            let undistributed_balance = if let Some(token_index) =
                self.reward_tokens.iter().position(|&t| t == token)
            {
                total_balance.saturating_sub(self.farm_distributed_unclaimed_rewards[token_index])
            } else {
                total_balance
            };
            token_ref.transfer(self.owner, undistributed_balance, vec![])?;
            Ok(())
        }

        // To learn how much rewards the user has, it's best to dry-run claim_rewards.
        #[ink(message)]
        fn claim_rewards(&mut self, tokens: Vec<u8>) -> Result<Vec<u128>, FarmError> {
            self.update()?;
            let account = self.env().caller();
            self.update_account(account);
            let mut user_rewards = self
                .user_claimable_rewards
                .get(account)
                .ok_or(FarmError::CallerNotFarmer)?;
            let mut rewards_claimed: Vec<(TokenId, u128)> = Vec::with_capacity(tokens.len());
            for token_idx in tokens {
                let idx = token_idx as usize;
                let token = self.reward_tokens[idx];
                let user_reward = user_rewards[idx];
                if user_reward > 0 {
                    user_rewards[idx] = 0;
                    let mut psp22_ref: ink::contract_ref!(PSP22) = token.into();
                    self.farm_distributed_unclaimed_rewards[idx] -= user_reward;
                    psp22_ref
                        .transfer(account, user_reward, vec![])
                        .map_err(|e| FarmError::TokenTransferFailed(token, e))?;
                    rewards_claimed.push((token, user_reward));
                }
            }
            if user_rewards.iter().all(|r| *r == 0) {
                self.user_claimable_rewards.remove(account);
            } else {
                self.user_claimable_rewards.insert(account, &user_rewards);
            }
            FarmContract::emit_event(
                self.env(),
                Event::RewardsClaimed(RewardsClaimed {
                    account,
                    rewards_claimed,
                }),
            );
            Ok(user_rewards)
        }

        #[ink(message)]
        fn withdraw(&mut self, amount: u128) -> Result<(), FarmError> {
            self.update()?;
            let account = self.env().caller();
            self.update_account(account);
            let shares = self.shares.get(account).unwrap_or(0);
            if let Some(new_shares) = shares.checked_sub(amount) {
                self.shares.insert(account, &new_shares);
                self.total_shares -= amount;
            } else {
                return Err(FarmError::InsufficientShares);
            }
            let mut pool: contract_ref!(PSP22) = self.pool_id.into();
            pool.transfer(account, amount, vec![])?;
            FarmContract::emit_event(self.env(), Event::Withdrawn(Withdrawn { account, amount }));
            Ok(())
        }

        #[ink(message)]
        fn view_farm_details(&self) -> FarmDetails {
            FarmDetails {
                pool_id: self.pool_id,
                start: self.start,
                end: self.end,
                reward_tokens: self.reward_tokens.clone(),
                reward_rates: self.farm_reward_rates.clone(),
            }
        }
    }

[Abbasjafri-syed]

This is the recommendation as unable to load on the site.

2. Revised Code File (Optional) Declare all function with pub that are meant to be public as having the #[ink(message)] attribute.

   
   impl Farm for FarmContract {
       #[ink(message)]
   -       fn pool_id(&self) -> AccountId {
   +       pub fn pool_id(&self) -> AccountId {
           self.pool_id
       }
       #[ink(message)]
   -        fn total_shares(&self) -> u128 {
   +        pub fn total_shares(&self) -> u128 {
   
           self.total_shares
       }
   
       #[ink(message)]
   -        fn shares_of(&self, owner: AccountId) -> u128 {
   +        pub fn shares_of(&self, owner: AccountId) -> u128 {
           self.shares.get(owner).unwrap_or(0)
       }
   
       #[ink(message)]
   -        fn reward_tokens(&self) -> Vec<AccountId> {
   +        pub fn reward_tokens(&self) -> Vec<AccountId> {
           self.reward_tokens.clone()
       }
   
       #[ink(message)]
   -        fn owner_start_new_farm(
   +       pub fn owner_start_new_farm(
           &mut self,
           start: Timestamp,
           end: Timestamp,
           rewards: Vec<u128>,
       ) -> Result<(), FarmError> {
           ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner);
           self.update()?;
           ensure!(!self.is_active(), FarmError::FarmAlreadyRunning);
           self.farm_reward_rates = self.assert_start_params(start, end, rewards.clone())?;
           self.start = start;
           self.end = end;
           Ok(())
       }
   
       #[ink(message)]
   -       fn owner_stop_farm(&mut self) -> Result<(), FarmError> {
   +       pub fn owner_stop_farm(&mut self) -> Result<(), FarmError> {
           ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner);
           self.update()?;
           self.end = self.env().block_timestamp();
           Ok(())
       }
   
       #[ink(message)]
   -       fn owner_withdraw_token(&mut self, token: TokenId) -> Result<(), FarmError> {
   +       pub fn owner_withdraw_token(&mut self, token: TokenId) -> Result<(), FarmError> {
           ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner);
           ensure!(!self.is_active(), FarmError::FarmAlreadyRunning);
           // Owner should be able to withdraw every token except the pool token.
           ensure!(self.pool_id != token, FarmError::RewardTokenIsPoolToken);
           self.update()?;
           let mut token_ref: contract_ref!(PSP22) = token.into();
           let total_balance = token_ref.balance_of(self.env().account_id());
           let undistributed_balance = if let Some(token_index) =
               self.reward_tokens.iter().position(|&t| t == token)
           {
               total_balance.saturating_sub(self.farm_distributed_unclaimed_rewards[token_index])
           } else {
               total_balance
           };
           token_ref.transfer(self.owner, undistributed_balance, vec![])?;
           Ok(())
       }
   
       // To learn how much rewards the user has, it's best to dry-run claim_rewards.
       #[ink(message)]
   -        fn claim_rewards(&mut self, tokens: Vec<u8>) -> Result<Vec<u128>, FarmError> {
   +        pub fn claim_rewards(&mut self, tokens: Vec<u8>) -> Result<Vec<u128>, FarmError> {
           self.update()?;
           let account = self.env().caller();
           self.update_account(account);
           let mut user_rewards = self
               .user_claimable_rewards
               .get(account)
               .ok_or(FarmError::CallerNotFarmer)?;
           let mut rewards_claimed: Vec<(TokenId, u128)> = Vec::with_capacity(tokens.len());
           for token_idx in tokens {
               let idx = token_idx as usize;
               let token = self.reward_tokens[idx];
               let user_reward = user_rewards[idx];
               if user_reward > 0 {
                   user_rewards[idx] = 0;
                   let mut psp22_ref: ink::contract_ref!(PSP22) = token.into();
                   self.farm_distributed_unclaimed_rewards[idx] -= user_reward;
                   psp22_ref
                       .transfer(account, user_reward, vec![])
                       .map_err(|e| FarmError::TokenTransferFailed(token, e))?;
                   rewards_claimed.push((token, user_reward));
               }
           }
           if user_rewards.iter().all(|r| *r == 0) {
               self.user_claimable_rewards.remove(account);
           } else {
               self.user_claimable_rewards.insert(account, &user_rewards);
           }
           FarmContract::emit_event(
               self.env(),
               Event::RewardsClaimed(RewardsClaimed {
                   account,
                   rewards_claimed,
               }),
           );
           Ok(user_rewards)
       }
   
       #[ink(message)]
   -        fn withdraw(&mut self, amount: u128) -> Result<(), FarmError> {
   +        pub fn withdraw(&mut self, amount: u128) -> Result<(), FarmError> {
           self.update()?;
           let account = self.env().caller();
           self.update_account(account);
           let shares = self.shares.get(account).unwrap_or(0);
           if let Some(new_shares) = shares.checked_sub(amount) {
               self.shares.insert(account, &new_shares);
               self.total_shares -= amount;
           } else {
               return Err(FarmError::InsufficientShares);
           }
           let mut pool: contract_ref!(PSP22) = self.pool_id.into();
           pool.transfer(account, amount, vec![])?;
           FarmContract::emit_event(self.env(), Event::Withdrawn(Withdrawn { account, amount }));
           Ok(())
       }
   
       #[ink(message)]
   -        fn view_farm_details(&self) -> FarmDetails {
   +        pub fn view_farm_details(&self) -> FarmDetails {
           FarmDetails {
               pool_id: self.pool_id,
               start: self.start,
               end: self.end,
               reward_tokens: self.reward_tokens.clone(),
               reward_rates: self.farm_reward_rates.clone(),
           }
       }
   }

[deuszx]

Duplicate of https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/issues/1.

[Abbasjafri-syed]

These function will not be accessible externally for users...the contract are designed to interact with users and absence of public modifier made them private.

Considering above scenario another contract will be required to inherit this contract to call all the functions making whole process more complex and expensive.

[deuszx]

They don't need to have pub modifier to be accessible from other contracts. There's no inheritance of contracts in Substrate either.

To sum up, this is intended and not diverging from any standard.

[deuszx]

Thank you for participation. After carefully reviewing the submission we've concluded that this issue is INVALID.

We hope you participate in the future audits of ink!.