Mira v1 Core & Periphery - Mira

In Mira's implementation of the get_y function, the method does not revert if it fails to converge within 255 iterations:

fn get_y(x_0: u256, xy: u256, y: u256) -> u256 {
// ... [iteration logic] ...
    while i < 255 {
// ... [convergence attempts] ...
        i += 1;
    }
    y// Returns potentially non-converged value
}

In contrast, Velodrome's implementation reverts if convergence is not achieved:

function _get_y(uint256 x0, uint256 xy, uint256 y) internal view returns (uint256) {
    for (uint256 i = 0; i < 255; i++) {
// ... [convergence attempts] ...
    }
    revert("!y");// Reverts if no convergence
}

The lack of reversion in Mira's implementation introduces a vulnerability. If the function fails to converge, it returns a potentially incorrect y value.

In the case of high volatility or extreme scenario, this can lead to:

1. Inaccurate price calculations.

2. Unfair trade executions.

3. Potential manipulation of pool balances.

4. Loss of funds for liquidity providers or traders.

It is recommended to modify Mira's get_y function to revert if convergence is not achieved.

fn get_y(
    x_0: u256,
    xy: u256,
    y: u256
) -> u256 {
    let mut y: u256 = y;

    let mut i = 0;
    while i < 255 {
        // ... // 
    }
    
    // If we reach here, convergence was not achieved
    assert(false, "Y not found");
    0 // This line is never reached due to the assert, but Sway requires a return value
}

RISK ACCEPTED: The Mira team wants to keep the Velodrome V1 format and will not implement a change, moreover risk is handled on core part.

The check_deadline function in the blockchain_utils library implements a strict inequality check for the deadline:

pub fn check_deadline(deadline: u32) {
    require(deadline > height(), "Deadline passed");
}

This implementation only allows transactions to be executed when the current block height is strictly less than the specified deadline. It does not allow transactions to be executed at the exact block height specified as the deadline.

This strict inequality check results in a loss of precision in deadline enforcement. Users intending to set a deadline for the exact block height N will find their transactions failing at block height N, even though they intended to allow execution up to and including that block. This behavior is inconsistent with common expectations and can lead to unnecessary transaction failures, particularly in time-sensitive operations or when precise control over execution timing is required.

It is recommended to modify the check_deadline function to use a greater than or equal to comparison:

pub fn check_deadline(deadline: u32) {
    require(deadline >= height(), "Deadline passed");
}

SOLVED: Non-strict check has been implemented by the Mira team.

The AMM contract implements a single-step ownership transfer function that allows immediate transfer of contract ownership without verification:

#[storage(read, write)]
fn transfer_ownership(new_owner: Identity) {
    if _owner() == State::Uninitialized {
        initialize_ownership(new_owner);
    } else {
        transfer_ownership(new_owner);
    }
}

This implementation lacks a crucial two-step verification process. The current owner can transfer ownership directly to a new address without requiring the new owner to accept the transfer.

These vulnerabilities can lead to complete loss of contract control, potential fund lockup, and inability to perform critical administrative functions.

It is recommended to implement a two-step ownership transfer process:

1. The current owner proposes a new owner.

2. The proposed owner must accept the ownership.

Example implementation:

pub struct Contract {
    owner: Identity,
    pending_owner: Option<Identity>,
}

impl Contract {
    #[storage(read, write)]
    fn transfer_ownership(new_owner: Identity) {
        require(msg.sender() == self.owner, "Not current owner");
        self.pending_owner = Some(new_owner);
    }

    #[storage(read, write)]
    fn accept_ownership() {
        require(Some(msg.sender()) == self.pending_owner, "Not pending owner");
        self.owner = msg.sender();
        self.pending_owner = None;
    }
}

This two-step process ensures that ownership is only transferred to an address that can actively accept it, significantly reducing the risk of accidental or malicious ownership loss.

ACKNOWLEDGED: As the Fuel standard still does not support 2-step ownership transfer, the Mira team decided to keep it as is for now.

The smart contract codebase exhibits frequent use of magic numbers, which are hardcoded numerical values without clear context or explanation. These occurrences are found across multiple files and functions. For example:

amounts_in.get(amounts_in.len() - i - 2)

0x3u256 * x_0

for i in range(255)

volatile_fee <= LP_FEE_VOLATILE / 5

These magic numbers lack clear semantic meaning and make the code less maintainable and more prone to errors during future modifications

Replace all magic numbers with named constants that clearly describe their purpose and meaning. This practice enhances code readability, maintainability, and reduces the likelihood of errors during future updates. For example:

const MAX_ITERATIONS: u256 = 255; for i in range(MAX_ITERATIONS)

const PROTOCOL_FEE_DIVISOR: u256 = 5; volatile_fee <= LP_FEE_VOLATILE / PROTOCOL_FEE_DIVISOR

ACKNOWLEDGED: The Mira team acknowledged the finding but will keep it as is to reproduce the same code as UniV2 and Velodrome V1.

The get_amounts_out and get_amounts_in functions in the libraries/math/src/pool_math.sw file contain while loops that iterate over the entire length of the pools array. This design allows for an arbitrarily large number of iterations, potentially leading to excessive gas consumption or transaction failures if the pools array is sufficiently large.

Specific instances:

while (i < pools.len()) {
    // ... (loop body)
    i += 1;
}

2. In the get_amounts_in function:

while (i < pools.len()) {
    // ... (loop body)
    i += 1;
}

In both cases, the number of loop iterations is directly tied to the size of the pools array. If this array becomes excessively large, it could lead to significant gas costs or even cause transactions to fail by exceeding the block gas limit.

To prevent potential infinite loops, it is recommended to add an explicit break condition based on a maximum number of iterations:

const MAX_ITERATIONS: u64 = 256;
let mut iteration_count: u64 = 0;
while (i < pools.len() && iteration_count < MAX_ITERATIONS) {
    // ... (loop body)
    i += 1;
    iteration_count += 1;
}
if iteration_count == MAX_ITERATIONS {
    revert("Maximum iteration count reached");
}

ACKNOWLEDGED: The Mira team decided to keep it as is because it is the user's responsibility to prove adequate input parameters.

The burn function contains a redundant call to validate_pool_id:

https://github.com/mira-amm/mira-v1-core/blob/e0afbeb56ba8c01e223f2552979ee55e461d24a9/contracts/mira_amm_contract/src/main.sw#L513

fn burn(pool_id: PoolId, to: Identity) -> (u64, u64) {
    reentrancy_guard();
    validate_pool_id(pool_id);

    ...
}

as it is already done when calling get_pool:

https://github.com/mira-amm/mira-v1-core/blob/e0afbeb56ba8c01e223f2552979ee55e461d24a9/contracts/mira_amm_contract/src/main.sw#L518

    fn burn(pool_id: PoolId, to: Identity) -> (u64, u64) {
        reentrancy_guard();
        validate_pool_id(pool_id);

        ...

        let mut pool = get_pool(pool_id);

        ...

fn get_pool(pool_id: PoolId) -> PoolInfo {
    let pool = get_pool_option(pool_id);
    require(pool.is_some(), InputError::PoolDoesNotExist(pool_id));
    pool.unwrap()
}

fn get_pool_option(pool_id: PoolId) -> Option<PoolInfo> {
    validate_pool_id(pool_id);
    storage.pools.get(pool_id).try_read()
}

Remove the first call to validate_pool_id.

SOLVED: The Mira team solved this issue.