# Aptos issues assessment

This document lists the possible security checks that could be incorporated into the Aptos Move Lint solution.

This list is the result of the review of approximately 30 security audits of Aptos projects, [Move security recommendations](https://aptos.dev/en/build/smart-contracts/move-security-guidelines) and CoinFabrik's team insights from its experience on static analysis tools.

Although the security audits presented a vast amount of relevant issues and vulnerabilities, our work focused on narrowing down to the issues that can be detected by static analysis techniques.

Note that this is not the definitive list of the security checks that will be added to Aptos Move Lint. However, it is intended to serve as a starting point for further refinement and assessment.

## Aptos projects security audits

### 1. Lack of slippage checks

- Project: [Thala](https://www.thalalabs.xyz/)
- Repo: https://github.com/ThalaLabs/thala-modules/thalaswap_v2 (Private)
- Audit: [thalaswap_v2_audit_final](https://drive.google.com/file/d/1bsPIgUDHCjsIiXV45-vPc2TORGGC4N1P/view?usp=drive_link)
- Issue: OS-THS-ADV-01
- Severity: High
- Type: TBC

#### Description

The vulnerability concerns the lack of slippage checks within the entry functions in pool. Slippage parameters protect the protocol from accepting values that are drastically different from the current market conditions due to market volatility or large trades in the pool. This can result in inaccurate transactions within the pool, potentially affecting users unfairly. This issue is especially relevant for the swap functionalities. While performing a swap, several factors, including the impact of trade size on the pool price, market volatility, and front-running attacks, may affect the asset prices in the pool. Any of these factors may result in the user receiving a reduced output swap amount compared to what was originally expected.

#### Code snippet

Unavailable

### 2. Double coin register

- Project: Pyth Network
- Repo: https://github.com/pyth-network/pyth-crosschain
- Audit: [Pyth Aptos Audit Report](https://drive.google.com/file/d/1xV45amNlHcrF0Qlj2oj5cM3vy6uAPja7/view?usp=drive_link)
- Issue: OS-PYA-ADV-00
- Severity: Low
- Type: DOS

#### Description

During initialization of the `pyth` module, it attempts to register an `AptosCoin` account in order to be able to receive fees:

module pyth::pyth {
...
fun init_internal(...) {
...
coin::register<AptosCoin>(&wormhole);
}
...
}

However, `coin::register` is a one-time operation. If `coin::register` has previously been called on this address, this initialization code will abort and the wormhole program will be unable to initialize.

While it is usually not possible to register coins for users you can not sign for, the Aptos framework provides a special mechanism to register AptosCoin for any user via `aptos_account::create_account`:

public entry fun create_account(auth_key: address) {
let signer = account::create_account(auth_key);
coin::register<AptosCoin>(&signer);
}

Therefore, with this mechanism an attacker could register AptosCoin for the wormhole program before deployment in order to prevent it from properly initializing.

### 3. Infinite loop

- Project: [BAPTASWAP](https://baptswap.com/)
- Repo: https://github.com/BAPTSWAP/V2-core
- Audit: [BAPTSWAP-Final-Audit-Report](https://drive.google.com/file/d/14BbPU7IKFcc6xmvU_KwLef3GqrZKjm3w/view?usp=drive_link)
- Issue: SV2-3
- Severity: Critical
- Type: Validations and error handling

#### Description

The function `swap_v2.distribute_dex_fees()` aims to calculate and distribute DEX fees based on the type of input X. In this function, the protocol calls `swap_exact_x_to_y_direct()` to exchange X for APT and then transfers the obtained APT to the treasury. However, within the `swap_exact_x_to_y_direct()` function, the protocol again invokes `distribute_dex_fees()`. This recursive calling pattern leads to an infinite loop, resulting in an out-of-gas situation and a failed transaction.

if (type_info::type_of<X>() != type_info::type_of<APT>()) {
    let metadata = borrow_global_mut<TokenPairMetadata<X, Y>>
(RESOURCE_ACCOUNT);
    /// extract it from balance x from the metadata
    let coin_x_out = coin::extract<X>(&mut metadata.balance_x, amount_in);
    /// update reserves
    update_reserves<X, Y>();
    /// swap it to APT
    let (coins_x_out, coin_y_out) = swap_exact_x_to_y_direct<X, APT>(coin_x_out);
    coin::destroy_zero(coins_x_out); /// or others ways to drop `coins_x_out`
    /// deposit APT to treasury
    let swap_info = borrow_global<SwapInfo>(RESOURCE_ACCOUNT);
    coin::deposit<APT>(swap_info.fee_to, coin_y_out);
}

### 4. Zero or test address

- Project: [BAPTASWAP](https://baptswap.com/)
- Repo: https://github.com/BAPTSWAP/V2-core
- Audit: [BAPTSWAP-Final-Audit-Report](https://drive.google.com/file/d/14BbPU7IKFcc6xmvU_KwLef3GqrZKjm3w/view?usp=drive_link)
- Issue: SV2-5
- Severity: Medium
- Type: Validations and error handling

#### Description

In the init_module function, initializing `fee_to` as `ZERO_ACCOUNT` means that if the `set_fee_to` function is called to set a new address for fee reception, swap fees will be transferred to the zero address.

fun init_module(sender : &signer) {
  let signer_cap = resource_account::retrieve_resource_account_cap(sender, DEV);
  let resource_signer = account::create_signer_with_capability(&signer_cap);
  move_to(&resource_signer, SwapInfo{
    signer_cap,
    fee_to : ZERO_ACCOUNT,
    admin : DEFAULT_ADMIN,
    liquidity_fee_modififier : 30,  /// 0.3%
    treasury_fee_modififier : 60,   /// 0.6%
    pair_created :
        account::new_event_handle<PairCreatedEvent>(&resource_signer),
  });
}

### 5. Inappropriate mutable borrow

- Project: [Mokshya Protocol](https://mokshya.io/)
- Repo: https://github.com/mokshyaprotocol/aptos-nft-random-mint
- Audit: [BAPTSWAP-Final-Audit-Report](https://drive.google.com/file/d/14BbPU7IKFcc6xmvU_KwLef3GqrZKjm3w/view?usp=drive_link)
- Issue: 6.3
- Severity: Medium
- Type: TBC

#### Description

In the function `candymachine::mint_script`, the resource `CandyMachine` is obtained through `borrow_global_mut`, but there is no need to modify `CandyMachine` in this function. Using `borrow_global_mut` may be risky, and the function `candymachine::mint_from_merkle` also has this problem. The same problem is similar to using `table_with_length::borrow_mut` in the function `candymachine::bucket_table::borrow`.

public entry fun mint_script(
    receiver: &signer,
    candymachine: address,
)acquires ResourceInfo, CandyMachine,MintData,PublicMinters{
    let candy_data = borrow_global_mut<CandyMachine>(candymachine);
    let mint_price = candy_data.public_sale_mint_price;
    let now = aptos_framework::timestamp::now_seconds();
    assert!(now > candy_data.public_sale_mint_time, ESALE_NOT_STARTED);
    mint(receiver,candymachine,mint_price)
}

public fun borrow<K: copy + drop, V>(map: &mut BucketTable<K, V>, key: K): &V
{
    let index = bucket_index(map.level, map.num_buckets, sip_hash_from_value(&
key));
    let bucket = table_with_length::borrow_mut(&mut map.buckets, index);
 ......
}

### 6. Unused function

- Project: [MoveDID](https://home.did.rootmud.xyz/)
- Repo: https://github.com/NonceGeek/MoveDID
- Audit: [MoveDID-Aptos-Contracts-Audit-Report](https://drive.google.com/file/d/1nC-BJXGstfrn5YSMtAsdWUMvmVopuQZQ/view?usp=drive_link)
- Issue: 5.3
- Severity: Minor
- Type: Best practices

#### Description

This function is not an entry function and has not been called.

### 7. Contains in table

- Project: [LayerZero](https://layerzero.network/)
- Repo: https://github.com/LayerZero-Labs/LayerZero-Aptos-Contract
- Audit: [LayerZero Private Audit Report](https://drive.google.com/file/d/1em4Poa-5AWgYIQLy-iAgSszNWxeEFcg9/view?usp=drive_link)
- Issue: OS-LZR-SUG-01
- Severity: Medium
- Type: Validation and error handling

#### Description

In some functions, `table::borrow` and `table::add` are used without error handling.
`table::borrow` will throw an error, if no value exists for the given key.
`table::add` will fail, if there exists a value for the given key already.

public entry fun send_to_remote(account: &signer, chain_id: u64, fee:
,→ u64) ... {
[..]
let config = borrow_global<Config>(signer_addr);
let dst_address = table::borrow(&config.trusted_remote, chain_id);
[..]
}
public entry fun set_remote(account: &signer, chain_id: u64, address:
,→ vector<u8>) .. {
[..]
let config = borrow_global_mut<Config>(signer_addr);
table::add(&mut config.trusted_remote, chain_id, address);
}

### 8. Concatenating before hash

- Project: [Wormhole](https://wormhole.com/)
- Repo: https://github.com/wormhole-foundation/wormhole/
- Audit: [Wormhole Aptos Audit](https://drive.google.com/file/d/1z4PsDHtA3SJFeKaOCeWcZnkjF13_ktlq/view?usp=drive_link)
- Issue: OS-WHA-ADV-00
- Issue found in commit: 5837c83
- Severity: Medium
- Type: Authorization

#### Description

In both `wormhole` and `token-bridge`, contract upgrade via governance is a two step process:

1. A user invokes `contract_upgrade::submit_vaa` with a governance VAA that includes a hash of the intended upgrade code.
2. A user invokes `contract_upgrade::upgrade` with the actual module code which subsequently invokes `code::publish_package_txn` to upgrade the contract.

Both of these calls are permissionless (can be invoked by any user). Integrity is protected in the first call by verifying the guardian signatures on the VAA. Integrity is protected in the second call by ensuring the provided module code matches the hash.

However, in the original implementation, we identified two issues that would allow an attacker to provide alternative module code that still matches the stored hash:

public entry fun upgrade(
metadata_serialized: vector<u8>,
code: vector<vector<u8>>
) acquires UpgradeAuthorized {
...
let c = copy code;
vector::reverse(&mut c);
let a = vector::empty<u8>();
while (!vector::is_empty(&c)) vector::append(&mut a,
,→ vector::pop_back(&mut c));
assert!(keccak256(a) == hash, E_UNEXPECTED_HASH);
...
}

A module upgrade package contains a list of code modules (`vector<vector<u8>>`) and serialized metadata (`vector<u8>`). In the original implementation, metadata_serialized was not included in the hash. Therefore, an attacker could provide any arbitrary metadata that would not cause the deployment to abort.

The original hash was computed by first concatenating the code modules and then taking the hash of the concatenated structure. However, this implementation does not properly validate module code boundaries. Specifically, moving N bytes of code from the end of one module to the start of another would
not affect the hash:

H([a,b,c,d], [e,f]) == H([a,b], [c,d,e,f])

It is unclear whether an attacker could exploit this collision to deploy a malformed version of the module. However, since the fix is simple, it is preferable to reduce the attack surface as much as possible.

### 9. Unused generics

- Project: [EconiaLabs](https://econialabs.com/)
- Repo: https://github.com/econia-labs/econia
- Audit: [Econia Audit Report](https://drive.google.com/file/d/1GzntbC4CC6vzqFxYXmCssJxnkHwxJ9ak/view?usp=sharing)
- Issue:
- Severity:
- Type:

CHECK!

### 10. Unchecked admin

- Project: [Meeiro](https://meeiro.xyz/)
- Repo: https://github.com/elmosqui/move-contracts/ (Private)
- Audit: [Meeiro Audit Report](https://drive.google.com/file/d/1tMmB3it3i3DbJMjzNVid33WsO0XAMv06/view?usp=drive_link)
- Issue: OS-MSK-SUG-00
- Severity: Medium
- Type: Authorization

#### Description

The protocol implements certain features which are only accessible to the admin, such as: `set_token_per_second`, `set_treasury`, etc. What makes these functions accessible only to the admin is the following check:

public entry fun set_token_per_second<StakeCoin, RewardCoin>(admin:
&signer, token_per_second: u64) acquires PoolInfo,
OwnerCapability {
,→
,→
let admin_address = signer::address_of(admin);
let pool_address = account::create_resource_address(&@MeeStaking,
,→ POOL_SEED);
check_admin(pool_address, admin_address);

Where the `check_admin` function asserts if the signer is not the admin set at initialization.

fun check_admin(pool_address: address, admin_address: address) acquires
    ,→ OwnerCapability {
    let owner_capability = borrow_global<OwnerCapability>(pool_address);
    // check if current signer is real admin
    assert!(owner_capability.admin_address == admin_address,
    ,→ error::permission_denied(EINVALID_ADMIN));
}

However, this function in not used in all of the functions which are meant to be accessed only by the admin, but the program implements the assert in the `set_treasury`, `add_pool` and `transfer_ownership` function directly.

### 11. Unused parameters

- Project: [Pancake Swap](https://meeiro.xyz/)
- Repo: https://github.com/elmosqui/move-contracts/ (Private)
- Audit: [PancakeSwap Audit Report](https://drive.google.com/file/d/1MK2WIGtZIVF8HxIuyPvYM7_J549xsvYw/view?usp=drive_link)
- Issue: OS-PAN-SUG-03
- Severity: Minor
- Type: Best Practices

#### Description

The `TokenPairMetadata` struct contains an unnecessary `lp` field. This field is used during `remove_liquidity` as an intermediary but serves no purpose and can be removed:

fun remove_liquidity_direct<X, Y>(
    liquidity: coin::Coin<LPToken<X, Y>>,
): (coin::Coin<X>, coin::Coin<Y>, u64) acquires TokenPairMetadata,
,→ TokenPairReserve {
    transfer_lp_coin_in<X, Y>(liquidity);
    let (coins_x, coins_y, fee_amount) = burn<X, Y>();
    (coins_x, coins_y, fee_amount)
}

### 12. Unnecesary wrapper function

- Project: [Tortuga](https://tortuga.finance/)
- Repo: https://github.com/Tortuga-Finance/tortuga-protocol (Private)
- Audit: [tortugal_tip_audit_final](https://drive.google.com/file/d/1Mxzh1Hs8C21IjUF2gl8cm6juu2xe618L/view?usp=drive_link)
- Issue: OS-TOR-SUG-01
- Severity: Minor
- Type: Best Practices

#### Description

In `stake_router.move`, `get_total_value_locked` simply wraps `get_total_worth`, which is unnecessary.

public fun get_total_value_locked(): u64 acquires StakingStatus {
    get_total_worth()
}

### 13. Incorrect admin transfer

- Project: [Streamflow Finance](https://streamflow.finance/)
- Repo: https://github.com/streamflow-finance/aptos-streamflow-module (Private)
- Audit: [streamflow_audit_final](https://drive.google.com/file/d/1ex0dupy2BqgfyKAKyiaRh2mh-iq7-ODj/view?usp=drive_link)
- Issue: OS-SFW-SUG-00
- Severity: Critical
- Type: Authorization

#### Description

In the `admin` module, `transfer_admin` transfers the ownership of the protocol configuration to `new_admin`.

public entry fun transfer_admin(account: &signer, new_admin: address)
    ,→ acquires ConfigV2 {
    let config = borrow_global_mut<ConfigV2>(@Streamflow);
    assert!(
    address_of(account) == config.admin,
    error::permission_denied(EADMIN_NOT_AUTHORIZED)
    );
    config.admin = new_admin;
}

### 14. Check oracle confidence

- Project: [Aries Markets](https://app.ariesmarkets.xyz/lending)
- Repo: https://github.com/aries-markets/aries-markets (Private)
- Audit: [aries_markets_audit_final](https://drive.google.com/file/d/193OPANI0-4JXcD1UM_XvFdfKyvyKIk4y/view?usp=drive_link)
- Issue: OS-SFW-SUG-00
- Severity: Minor
- Type: Best Practices

The `oracle::get_pyth_price` function currently retrieves the latest price from the [Pyth oracle](https://www.pyth.network/pyth-price-feeds) without explicitly considering the confidence deviation. To enhance the reliability and accuracy of the prices obtained, it is advisable to validate that Pyth’s confidence deviation is less than 10% of the retrieved price. This validation can be incorporated directly within the `get_pyth_price` function to ensure that the price data used is within an acceptable range of certainty.

### 15. Modulo bias in random generators

- Project: Emojicoin
- Repo: https://github.com/econia-labs/emojicoin-dot-fun (Private)
- Audit: [emojicoin_audit_final](https://drive.google.com/file/d/1mK98pwDicm5ZJB_RFjA177Yb2-UyW87d/view?usp=drive_link)
- Issue: OS-EMJ-SUG-00
- Severity: Minor
- Type: Best Practices

#### Description

The use of the modulo operator `(%)` in `u64_range` introduces a bias that makes the output non-uniformly random, potentially creating predictable patterns. When using `%` range to map a large number space (`u256`) to a smaller one (`u64` range), the distribution becomes uneven if the range does not evenly divide the maximum possible value of `u256`. This occurs because some numbers in the `u256` space wrap around and end up with a slightly higher probability of being selected. Care must be taken to ensure there is no unintended bias in the random generation process.

/// Pseudo-random substitute for `aptos_framework::randomness::u64_range`, since
/// the randomness API is not available during `init_module`.
public(friend) inline fun u64_range(min_incl: u64, max_excl: u64): u64 {
    let range = ((max_excl - min_incl) as u256);
    let sample = ((u256_integer() % range) as u64);
    min_incl + sample
}

### 16. Front-running

- Project: Emojicoin
- Repo: https://github.com/econia-labs/emojicoin-dot-fun (Private)
- Audit: [emojicoin_audit_final](https://drive.google.com/file/d/1mK98pwDicm5ZJB_RFjA177Yb2-UyW87d/view?usp=drive_link)
- Issue: OS-EMJ-ADV-00
- Severity: High
- Type: MEV

#### Description

There exists a potential for front-running when matched funds are allocated within the Emojicoin arena module. This vulnerability arises from the mechanism used to distribute matched amounts. Specifically, the module allows users to lock in a portion of their contribution to receive matched funds from the vault. An attacker can exploit this by creating numerous pools with small amounts, thereby increasing the likelihood that one of their pools is selected during the crank scheduling process.

Prior to the crank selecting a melee, the attacker may purchase a significant amount of their own token, artificially inflating its price and, consequently, its value relative to other tokens in the pool. If their pool is subsequently selected, the attacker can then buy into the pool and swap out their tokens to capture the matched funds.

Check https://aptos.dev/en/build/smart-contracts/move-security-guidelines#front-running

### 17. Empty strings

- Project: Aptos Securitize
- Repo: https://github.com/aptos-labs/aptos-securitize (Private)
- Audit: [aptos_securitize_audit_final](https://drive.google.com/file/d/1WbkH-9Rqq1jEFpAkH8l_VLcvv0x-5FxU/view?usp=drive_link)
- Issue: OS-ASC-ADV-15
- Severity: Low
- Type: Validations and error handling

#### Description

In `registry_service::register_investor`, an empty string used as an ID may be treated differently from other valid IDs. This is problematic because the contract assigns a special meaning to the empty string, and allowing it as an investor ID can disrupt various parts of the logic.

For example:

- If an investor registers with an empty string as their ID, the `is_wallet` function will return `true` for any wallet, as if the investor does not exist.
- Similarly, `get_investor` will return `""`, leading to ambiguity.

This inconsistency creates confusion within the contract logic, where functions that check for investor existence may incorrectly treat an empty ID as valid. Proper input validation should be enforced to reject empty strings as valid investor IDs.

/// Registers a new investor with the given ID and collision hash.
/// Only callable by issuer or transfer agent or above.
public entry fun register_investor(
    authorizer: &signer,
    id: String,
    collision_hash: String
) acquires InvestorInfo {
    trust_service::assert_only_issuer_or_transfer_agent_or_above(authorizer);
    assert!(!is_investor(id), EINVESTOR_ALREADY_EXISTS);

    let investor = Investor {
        id,
        collision_hash,
        creator: signer::address_of(authorizer),
        last_updated_by: signer::address_of(authorizer),
        country: string::utf8(b""),
        wallet_count: 0,
        wallets: vector::empty()
    };

    // [...]
}

### 18. Integer overflow or underflow

- Project: Aptos Securitize
- Repo: https://github.com/aptos-labs/aptos-securitize (Private)
- Audit: [aptos_securitize_audit_final](https://drive.google.com/file/d/1WbkH-9Rqq1jEFpAkH8l_VLcvv0x-5FxU/view?usp=drive_link)
- Issue: OS-ASC-ADV-09
- Severity: Critical
- Type: Arithmetic

#### Description

In `get_compliance_transferable_tokens_deposit` and `get_compliance_transferable_tokens` within `compliance_service`, difference is calculated as `time - lock_time` without first checking
if `time` is greater than or equal to l`ock_time`. If `lock_time` exceeds `time`, the subtraction operation (`time - lock_time`) will result in an underflow, causing the program to abort.

fun get_compliance_transferable_tokens_deposit(
    who: address,
    time: u64,
    lock_time: u64,
    from_balance: u64
): u64 acquires ComplianceData {
    assert!(time > 0, EINVALID_TIME);
    [...]

    let total_tokens_locked = 0;
    for (i in 0..investor_issuances_count) {
        let issuances_timestamps = &compliance_data.issuances_timestamps;
        let issuances_timestamps_inner =
            smart_table::borrow(issuances_timestamps, investor);
        let issuance_time =
            *smart_table::borrow_with_default(issuances_timestamps_inner, i, &0);
        let difference = time - lock_time;

        if (lock_time > time || issuance_time > (difference as u256)) {
            let issuances_values = &mut compliance_data.issuances_values;
            let issuances_values_inner =
                smart_table::borrow_mut(issuances_values, investor);
            total_tokens_locked = total_tokens_locked
                + *smart_table::borrow_mut_with_default(issuances_values_inner, i, 0);
        }
    };
    [...]
}

## Move security guidelines

The following are Move securitu recommendations that could be integrated into static analysis detectors.

1. Access Control - Object Ownership Check
2. Access Control - Function visibility
3. Arithmetic Operations/Division Precision
4. Arithmetic Operations/Integer Considerations
5. Aptos Objects/ConstructorRef leak
6. Randomness/test-and-abort