User can bypass the fees limits

[hats-bug-reporter[bot]]

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

Description: fn new allows for setting trade_fee & protocol_fee:

    pub fn new(trade_fee: u32, protocol_fee: u32) -> Option<Self> {
->        if trade_fee > MAX_TRADE_FEE || protocol_fee > MAX_PROTOCOL_FEE {
            None
        } else {
            Some(Self {
                trade_fee,
                protocol_fee,
            })
        }
    }

A maximum limit is established to ensure that fees cannot exceed the cap.

This function is called inside new_stable & new_rated:

        pub fn new_stable(
            tokens: Vec<AccountId>,
            tokens_decimals: Vec<u8>,
            init_amp_coef: u128,
            owner: AccountId,
            trade_fee: u32,
            protocol_fee: u32,
            fee_receiver: Option<AccountId>,
        ) -> Result<Self, StablePoolError> {
            let token_rates = vec![TokenRate::new_constant(RATE_PRECISION); tokens.len()];
            Self::new_pool(
                tokens,
                tokens_decimals,
                token_rates,
                init_amp_coef,
                owner,
->                Fees::new(trade_fee, protocol_fee),
                fee_receiver,
            )
        }

     pub fn new_rated(
            tokens: Vec<AccountId>,
            tokens_decimals: Vec<u8>,
            external_rates: Vec<Option<AccountId>>,
            rate_expiration_duration_ms: u64,
            init_amp_coef: u128,
            owner: AccountId,
            trade_fee: u32,
            protocol_fee: u32,
            fee_receiver: Option<AccountId>,
        ) -> Result<Self, StablePoolError> {
//..Ommitted code
            });
            Self::new_pool(
                tokens,
                tokens_decimals,
                token_rates,
                init_amp_coef,
                owner,
->                Fees::new(trade_fee, protocol_fee),
                fee_receiver,
            )
        }

The problem however, is that it is not checked inside new_pool:

        pub fn new_pool(
            tokens: Vec<AccountId>,
            tokens_decimals: Vec<u8>,
            token_rates: Vec<TokenRate>,
            amp_coef: u128,
            owner: AccountId,
            fees: Option<Fees>,
            fee_receiver: Option<AccountId>,
        ) -> Result<Self, StablePoolError> {
//..Ommitted code
            Ok(Self {
                ownable: Ownable2StepData::new(owner),
                pool: StablePoolData {
                    tokens,
                    reserves: vec![0; token_count],
                    precisions,
                    token_rates,
                    amp_coef,
->                    fees: fees.ok_or(StablePoolError::InvalidFee)?,
                    fee_receiver,
                },
                psp22: PSP22Data::default(),
            })
        }

Instead, it only verifies that the fees (trade_fee and protocol_fees) are not zero. If they are zero, the transaction is reverted; otherwise, it proceeds.

This means that users are able to bypass the set limits by calling new_pool directly.

Recommendation

Ensure that the check is performed inside new_pool, allowing you to omit the checks within the stable and rated functions, as they already call new_pool.

                pool: StablePoolData {
                    tokens,
                    reserves: vec![0; token_count],
                    precisions,
                    token_rates,
                    amp_coef,
-                    fees: fees.ok_or(StablePoolError::InvalidFee)?,
+                    fees::new(trade_fee, protocol_fee)
                    fee_receiver,
                },
                psp22: PSP22Data::default(),
            })
        }

Additionally the 0 check can be implemented inside fn new.

[JanKuczma]

Thank you for your submission.

new_stable(...) and new_rated(...) are the only constructors of this contract. new_pool(...) cannot be used when deploying a new instance of this contract. Fees::new(...) returns None when fees are too high. Both constructors use new_pool(...) which throws an error when fees (Fees:new(...)) is None.