Protocol MultiversX - Hatom

In the function update_rewards_batch_speed, there is a possibility that the calculated rewards batch period dt could be zero. This may lead to unexpected behavior, such as the rewards batch being stuck in the current state and preventing further rewards distribution.

The problem arises from the fact that there is no explicit check for whether dt is zero, and the code proceeds to update the rewards_batch.end_time with the potentially zero value.

Consider the following scenario:

A rewards batch has an initial speed of 100, old_dt of 100, and new_speed of 10,000.

The calculation for new_dt would then be: $$(100 100 wad) / (10,000 * wad) = 1$$

In this case, the value of dt is 1, which is greater than zero and proceeds normally.

However, if we change the initial speed to 1 and the new_speed to 10,000, the calculation for new_dt would result in:

$$ (1 100 wad) / (10,000 * wad) = 0.01 $$

Since the dt is being converted to u64, the fractional part is truncated, resulting in a value of 0 for dt.

Code Location

Down below is the code snippet from the update_rewards_batch_speed function:

• controller/src/governance.rs

let dt = match BigUint::to_u64(&new_dt) {
    None => sc_panic!(ERROR_UNEXPECTED_REWARDS_BATCH_PERIOD),
    Some(dt) => dt,
};

To mitigate this issue, it is recommended to add an explicit check for a zero value of dt and handle it appropriately. This might include returning an error or requiring a minimum rewards batch period.

Remediation Plan

SOLVED: The issue was solved in commit a76fa by adding a check to verify the reward's period is greater than zero when updating rewards speed.

The current ownership transfer process involves the approval of a proposal. When the proposal is successful, the admin is set to the proposed address in the try_change_admin function. This function checks if the new owner is not the zero address and proceeds to write the new admin. If the submitted account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking the access control in the execute endpoint and in the proposals' creation process. Lack of a two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take the administration of the contract.

Code Location

Down below is the code snippet from the try_change_admin function:

• governance/src/config.rs

fn try_change_admin(&self, address: ManagedAddress) {
        require!(!address.is_zero(), ERROR_ADDRESS_ZERO);
        self.admin().set(&address);
    }

It is recommended to introduce a validation step to confirm the new administrator address before updating the admin field, for example, requiring the new administrator to accept the role explicitly, potentially through a function call from the new admin address.

Code Location

This issue was solved in commit 1804e by requiring the new admin to accept their role to finalize the role transfer. It is important to note that the governance contract was moved to a new repository.

The anchor tolerance is set to ensure the acceptable deviation between the oracle's reported price and the actual market price of the asset. In the oracle's init function, this deviation is set using the parameter anchor_tolerance. The function set_anchor_tolerance_internal checks that the lower_bound is at least 1, but the upper bound is 1 + the submitted anchor tolerance, so the upper bound can have any value.

Big upper bounds could make the oracle vulnerable to price manipulations and/or high volatility.

Code Location

Down below is the code snippet from the set_anchor_tolerance_internal function:

The upper bound is calculated here:

• oracle/src/common.rs

fn set_anchor_tolerance_internal(&self, anchor_tolerance: &BigUint) {
        let wad = BigUint::from(WAD);
        let upper_bound = &wad + anchor_tolerance;
        let lower_bound = if anchor_tolerance < &wad {
            &wad - anchor_tolerance
        } else {
            // avoid prices being zero
            BigUint::from(1u32)
        };
        self.upper_bound_ratio().set(upper_bound);
        self.lower_bound_ratio().set(lower_bound);
    }

It is recommended to set a max value for the upper bound, so the anchor tolerance is always between a reasonable value.

Remediation Plan

SOLVED: The issue was solved in commit 035ec by setting a max_anchor_tolerance value as threshold.

In the borrow_internal function, there is a redundant check for underlying_amount > BigUint::zero(). This check is unnecessary as the borrow function, which calls borrow_internal, has already ensured that the underlying_amount is greater than zero. Removing this redundant check can improve code efficiency.

Code Location

Down below is the code snippet from the borrow_internal function:

• /money-market/src/borrow.rs

if borrower_current_borrow_amount == BigUint::zero() && underlying_amount > BigUint::zero() {
    self.borrower_count().update(|_count| *_count += 1u64);
}

To address this issue, it is recommended to remove the unnecessary check for underlying_amount > BigUint::zero() in the borrow_internal function.

Remediation Plan

SOLVED: The issue was solved in the commit 47658 by removing the underlying_amount > BigUint::zero() check.

The get_interest_rate_model_proxy function does not validate if the interest rate model address is defined before accessing it, which may lead to unexpected behavior if the address is not set, or has been accidentally removed or invalidated.

Code Location

Down below is the code snippet from the get_interest_rate_model_proxy function:

• /money-market/src/proxies.rs

fn get_interest_rate_model_proxy(&self, sc_address: Option<ManagedAddress>) -> interest_rate_model::Proxy<Self::Api> {
        match sc_address {
            Some(interest_rate_model_address) => self.interest_rate_model_proxy(interest_rate_model_address),
            None => {
                let interest_rate_model_address = self.interest_rate_model().get();
                self.interest_rate_model_proxy(interest_rate_model_address)

To mitigate this vulnerability, it is recommended to add a check before accessing the interest rate model to ensure that it is defined and valid.

Remediation Plan

SOLVED: The issue was solved in the commit ac0fc by adding a check to verify if the interest rate model contract has been defined.

It was observed that it is not possible to remove a supported pair from the whitelisted_maiar_pairs nor the whitelisted_chainlink_pairs. The only way to remove those pairs is overwriting its value, or pausing the market.

It is recommended to write a function to empty values from the whitelisted_maiar_pairs or to pause the use of supported pairs.

Remediation Plan

The issue was solved in commit 1eacfc by introducing the removeToken endpoint, which deletes all information related to a previously supported Token. The Hatom Protocol now places more emphasis on the token itself, rather than on pairs.