MoveBit Completes Security Audit for Concentrated Liquidity Protocol Aptos
MoveBit completed the security audit report on the Cetus Concentrated Liquidity Protocol Aptos. We are delighted to share this report with the community, you can download the PDF version of the security audit report through the link below: https://movebit.xyz/file/Cetus-Concentrated-Liquidity-Protocol-Aptos-Audit-Report.pdf
Here are the main roles in the smart contract with their respective capabilities:
(1) Protocol Admin
- Protocol Admin can transfer protocol authority to others.
- Protocol Admin can maintain the
fee_tier. - Protocol Admin can maintain the
partner. - Protocol Admin can maintain the
rewarder. - Protocol Admin can pause/unpause the
protocol. - Protocol Admin can pause/unpause the
pool.
(2) User
- User can create a new
pool. - User can add liquidity to a
pool. - User can remove liquidity from a
pool. - User can collect fees from a
pool. - User can collect rewards from a
pool. - User can trade on a
pool.
Methodology
The security team adopted the “Testing and Automated Analysis” , “Code Review” and “ Formal Verification” strategy to perform a complete security test on the code in a way that is closest to the real attack. The main entrance and scope of security testing are in the conventions in the “Audit Objective”, and that can expand to the context beyond the scope according to the actual testing needs. The main types of this security audit include:
(1) Testing and Automated Analysis
Items to check: state consistency/failure rollback/unit testing/value overflows/parameter verification / unhandled errors/boundary checking/coding specifications.
(2) Code Review
The following are the SHA1 hashes of the last reviewed files.
(3) Formal Verification
Perform formal verification for key functions with the Move Prover.
(4) Audit Process
- Carry out relevant security tests on the testnet or the mainnet;
- If there are any questions during the audit process, communicate with the code owner in time, and they should actively cooperate (which may include the latest stable source code, relevant deployment scripts or methods, transaction signature scripts, exchange docking schemes, etc.);
- The necessary information during the audit process will be well documented for both the audit team and the code owner in time.
Findings
- Invalid
end_timeargument ofpartner::create_partnermay causepartner::get_ref_fee_rateto return incorrect fee rate
Severity: Minor
Status: Fixed
Descriptions: partner::create_partnerdoesn't check whether the argument end_timeis greater than now. It is used to initialize the PartnerMetadata.end_time.If the PartnerMetadata.end_time is less than now, and not updated by partner::update_timelater, the partner would always get a zero fee rate returned by partner::get_ref_fee_rate, and thus the partner would never receive any partner fee.
Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/partner.move, line 141
public(friend) fun create_partner(
account: &signer,
name: String,
5/18
fee_rate: u64,
receiver: address,
start_time: u64,
end_time: u64,
) acquires Partners {
assert!(end_time > start_time, error::aborted(EINVALID_TIME));
assert!(fee_rate < MAX_PARTNER_FEE_RATE, error::invalid_argument(EINVALID_PARTNER_FEE_RATE));
......Suggestion: Refer the partner::update_time, adding an assertion statement like this assert!(end_time > timestamp::now_seconds(), error::aborted(EINVALID_TIME));at the beginning of this function.
2. The argument current_timeofpartner::get_ref_fee_rate may not be the current time
Severity: Medium
Status: Fixed
Descriptions: partner::get_ref_fee_rateis a public function, so everyone can call it. It returns the fee rate based on the input argument current_time. It doesn't check current_timeto match the current time, so the caller can decide what time to pass in to get more benefits. Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8 Code Location: sources/partner.move, line 289
public fun get_ref_fee_rate(name: String, current_time: u64): u64 acquires Partners {
let partners = &borrow_global<Partners>(@cetus_clmm).data;
if (!table::contains(partners, name)) {
return 0
};
let partner = table::borrow(partners, name);
if (partner.metadata.start_time > current_time || partner.metadata.end_time <= current_time) {
return 0
};
partner.metadata.fee_rate
}Suggestion: Remove the current_timeargument, and get the current time by calling timestamp::now_seconds()instead.
public fun get_ref_fee_rate(name: String): u64 acquires Partners {
let current_time = timestamp::now_seconds();
......
}3. Some test cases failed
Severity: Minor
Status: Fixed
Descriptions: While running the test cases, some failed in thepoolmodule. For example, the test_swapcase failed. In module clmm_math, the test_get_next_price_a_downshould be renamed to test_get_next_price_b_downas it tests get_next_sqrt_price_b_down. Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/pool.move, sources/math/clmm_math.move Suggestion: Fixing the test cases.
4 tick_math::get_sqrt_price_at_tickdoes not check whether the tick is in the range
Severity: Medium
Status: Fixed
Descriptions: Some ticks are out of range, but still work, for example, assert!(get_sqrt_price_at_tick(i64::neg_from(443637)) < 4295048016u128, 6)will not abort although it's out of the tick range . These cases may cause incorrect impacts.
Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/math/tick_math.move, line 30
Suggestion: Check whether the tick is in range, and abort if it's out of range.
public fun get_sqrt_price_at_tick(tick: i64::I64): u128 {
assert!(i64::gte(tick, min_tick()) && i64::lte(tick, max_tick()), EINVALID_TICK);
if (i64::is_neg(tick)) {
get_sqrt_price_at_negative_tick(tick)
} else {
get_sqrt_price_at_positive_tick(tick)
}
}5. Difference between get_delta_aand get_delta_bin module clmm_math
Severity: Minor
Status: Fixed
Descriptions: The get_delta_bfunction has the below check, but get_delta_adoes not.
if ((sqrt_price_diff == 0) || (liquidity == 0)) {
return 0
};Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/math/clmm_math.move, line 55
Suggestion: Adding the checks below.
public fun get_delta_a(
sqrt_price_0: u128,
sqrt_price_1: u128,
liquidity: u128,
round_up: bool
): u64 {
let sqrt_price_diff = if (sqrt_price_0 > sqrt_price_1) {
sqrt_price_0 - sqrt_price_1
} else {
sqrt_price_1 - sqrt_price_0
};
if (sqrt_price_diff == 0 || liquidity == 0) {
return 0
};
......
}6. partnerand fee_tiermodules don't have any functions to remove partner and fee
Severity: Minor
Status: Fixed
Descriptions: As time goes on, the partner and fee_tiermay have a large number of partners and fee_tiers. For administration, may need a way to remove the unused partnersand fee_tiers.
Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/partner.move, sources/fee_tier.move
Suggestion: Adding aremovefunction for partnerand fee_tiermodule.
7. router:: create_poolcan create a pool with the same type
Severity: Medium
Status: Fixed
Descriptions: router::create_pool<CoinA, CoinA>(...)can succeed. The swap between CoinAand CoinAis nonsense.
Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/router.move, line 161
Suggestion: When calling create_poolwith the same coin pair, abort.
8. Some assertions can be optimized
Severity: Medium
Status: Fixed
Descriptions: Many assertions for the input argument checks are not placed at the beginning of functions. It’s suggested that we should put them at the beginning of functions, so they can fail fast, and more gas-saving. For example, create_partner & update_fee_rateinpartner.move, and add_fee_tier&update_fee_tierin fee_tier.move.
Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Suggestion: Put argument check assertions at the beginning of functions.
9. Wrong event type emitted in factory::create_pool
Severity: Medium
Status: Fixed
Descriptions: In factory::create_pool, it emits CreatePoolEvent.coin_type_bwith CoinTypeA type. It's not correct, and it should be CoinTypeBtype.
Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/factory.move, line 123
Suggestion: ChangeCreatePoolEvent.coin_type_bwith CoinTypeB type.
public fun create_pool<CoinTypeA, CoinTypeB>(
......
event::emit_event(&mut pools.create_pool_events, CreatePoolEvent {
coin_type_a: type_of<CoinTypeA>(),
coin_type_b: type_of<CoinTypeB>(),
......
}10. Everyone can reset the initial price of a pool
Severity: Major
Status: Fixed
Descriptions: Everyone can reset the initial price of a pool by calling the public functionpool::reset_init_price. pool::reset_init_priceonly checks whether position_indexis equal to 1, that's not safe. Suppose in such a case, someone calls factory::create_poolto create a pool, and factory::create_poolwill create a default position with index 0 for him, and he can add liquidity to that position and produce some liquidity. At this time, someone else can reset the pool's initial price to another price successfully, and even trade the creator's assets at a lower price. In that case, it causes the creator's loss of assets.
Commit: c867755da203332468a37535c45ed7a7a4bbc65a
Code Location: sources/pool.move, line 436
Suggestion: Don't let everyone call this function, just leave it to the admin of the pool.
11. The comments on functions are out of date
Severity: Minor
Status: Fixed
Descriptions: Many function comments are out of date. For example, there is no argument named namein router::add_liquidity, router::add_liquidity_fix_token, router::remove_liquidity, androuter::collect_rewarder.
#[cmd]
/// Add liquidity into a pool. The position is identified by the name.
/// The position token is identified by (creator, collection, name), the creator is pool address.
/// Params
/// Type:
/// - CoinTypeA
/// - CoinTypeB
/// - pool
/// - delta_liquidity
/// - max_amount_a: the max number of coin_a can be consumed by the pool.
/// - max_amount_b: the max number of coin_b can be consumed by the pool.
/// - tick_lower
/// - tick_upper
/// - is_open: control whether or not to create a new position or add liquidity on existed position.
/// - name: position name. if `is_open` is true, name is no use.
/// Returns
public entry fun add_liquidity<CoinTypeA, CoinTypeB>(
account: &signer,
pool_address: address,
delta_liquidity: u128,
max_amount_a: u64,
max_amount_b: u64,
tick_lower: u64,
tick_upper: u64,
is_open: bool,
index: u64,
) {Commit: 25d115473799a9db777837553bd5e78bf88ca03a
Code Location: sources/router.move
Suggestion: Update the comments.
12. pool::add_liquidity_fix_coin& pool::add_liquidity have many duplicated codes
Severity: Minor
Status: Fixed
Descriptions: These two functions are very important to add liquidity, but they have 80% duplicated codes, which can be wrapped into a common function, and improve the code maintainability.
Commit: c867755da203332468a37535c45ed7a7a4bbc65a
Code Location: sources/pool.move
Suggestion: Refactoring these two functions, and wrapping the common codes into a new function.
13. pool::remove_liquiditydoes not callpool::update_rewarder
Severity: Critical
Status: Fixed
Descriptions: pool::update_rewarder is used to update the growth_globalupon swap, add liquidity, remove liquidity, collect rewarder and update emission. But pool::remove_liquidity does not call this function, it would cause the reward cumulative error.
Commit: c867755da203332468a37535c45ed7a7a4bbc65a
Code Location: sources/pool.move, 747
Suggestion: Update the codes, and call pool::update_rewarder.
14. Gas cost is higher than other DEX
Severity: Minor
Status: Pending
Descriptions: We testedcreate_pool, add_liquidityand swapin module clmm_router, and we found the average gas consumption for these operations is 0.0nlevel. This is somehow higher than other AMM DEX. As a CLMM DEX, Cetus definitely will have higher gas, and we already found some gas-optimization issues which Cetus has already taken, but Cetus still should improve to reduce the gas.
Suggestion: Keep reducing the gas for users.
15. utils::str optimization
Severity: Medium
Status: Fixed
Descriptions: The current implementation of utils::str is not optimized. It uses a pre-defined map to convert a u8 to a char and inserts the char into the index 0 of the string. This is very inefficient.
Commit: e56d47667850dbc5a9553eddb0f67572e7c3c3b8
Code Location: sources/utils.move, line 7
public fun str(num: u64): String {
let ns = simple_map::create<u64, String>();
simple_map::add(&mut ns, 1, string::utf8(b"1"));
simple_map::add(&mut ns, 2, string::utf8(b"2"));
simple_map::add(&mut ns, 3, string::utf8(b"3"));
simple_map::add(&mut ns, 4, string::utf8(b"4"));
simple_map::add(&mut ns, 5, string::utf8(b"5"));
simple_map::add(&mut ns, 6, string::utf8(b"6"));
simple_map::add(&mut ns, 7, string::utf8(b"7"));
simple_map::add(&mut ns, 8, string::utf8(b"8"));
simple_map::add(&mut ns, 9, string::utf8(b"9"));
simple_map::add(&mut ns, 0, string::utf8(b"0"));
if (num == 0) {
return string::utf8(b"0")
};
let res = string::utf8(b"");
let remainder ;
while (num > 0) {
remainder = num % 10;
num = num / 10;
string::insert(&mut res, 0, *simple_map::borrow<u64, String>(&ns, &remainder));
};
res
}Suggestion: Refer the implementation below.
public fun str2(num: u64): String {
if (num == 0) {
return string::utf8(b"0")
};
let remainder: u8;
let digits = vector::empty<u8>();
while (num > 0) {
remainder = (num % 10 as u8);
num = num / 10;
vector::push_back(&mut digits, remainder + 48);
};
vector::reverse(&mut digits);
string::utf8(digits)
}Gas cost comparison between strand str2:
16. Deploy smart contract without multi-sig
Severity: Medium
Status: Pending
Descriptions: The smart contract is not deployed under a multi-sig account. Operations performed with multiple signatures will provide greater security. Even if the loss of a single private key will not allow an attacker to gain access to the contract. Multiple trusted parties must approve the update at the same time, otherwise, it will not work.
Suggestion: Use a multi-sig account for the smart contract when deploying.
17. TODO labels still remain in the code
Severity: Minor
Status: Fixed
Descriptions: There are some TODOlabels in clmm_math.move, all the left TODOlabels are about tests. TODOoften means work is not finished or possibility of defects. If we're not sure about the codes, we should write more tests to ensure the codes work correctly.
Commit: 25d115473799a9db777837553bd5e78bf88ca03a
Code Location: source/math/clmm_math.move
#[test]
fun test_get_next_price_a_up() {
// TODO: Add more test for get_next_sqrt_price_a_up
......
}
#[test]
fun test_get_next_price_b_down() {
// TODO: Add more test for test_get_next_price_a_down
......
}
#[test]
fun test_compute_swap_step() {
// TODO: Add more test for test_compute_swap_step
......
}Suggestion: Add more test codes to ensure the correctness of codes.
18. Position recalculation optimization
Severity: Medium
Status: Fixed
Descriptions: In collect_fee and collect_rewarder functions in pool module, there are duplicated codes to get the pool and position. The reason is get_position_tick_range can not borrow the Pool resource after the poolvariable keeps a mutable reference to the Pool resource. This is a limitation of Move language to ensure security. We can solve this by introducing a helper function which uses a &Pool parameter to get the position tick range.
Commit: c867755da203332468a37535c45ed7a7a4bbc65a
Code Location: sources/pool.move, line 947
public fun collect_rewarder<CoinTypeA, CoinTypeB, CoinTypeC>(
account: &signer,
pool_address: address,
position_index: u64,
rewarder_index: u8,
recalculate: bool,
): Coin<CoinTypeC> acquires Pool {
check_position_authority<CoinTypeA, CoinTypeB>(account, pool_address, position_index);
let (pool, position) = if (recalculate) {
let (tick_lower, tick_upper) = get_position_tick_range<CoinTypeA, CoinTypeB>(pool_address, position_index);
let pool = borrow_global_mut<Pool<CoinTypeA, CoinTypeB>>(pool_address);
assert_status(pool);
update_rewarder(pool);
let rewards_growth_inside = get_reward_in_tick_range(pool, tick_lower, tick_upper);
let position = table::borrow_mut(&mut pool.positions, position_index);
update_position_rewarder(position, rewards_growth_inside);
(pool, position)
} else {
let pool = borrow_global_mut<Pool<CoinTypeA, CoinTypeB>>(pool_address);
assert_status(pool);
update_rewarder(pool);
let position = table::borrow_mut(&mut pool.positions, position_index);
(pool, position)
};
......
}Suggestion: Add a new function get_position_tick_range_by_pool to use a &Pool parameter. Then we can rewrite the collect_rewarder and collect_fee functions to remove the duplicated code.
// add this new function
public fun get_position_tick_range_by_pool<CoinTypeA, CoinTypeB>(
pool_info: &Pool<CoinTypeA, CoinTypeB>,
position_index: u64
): (I64, I64) {
if (!table::contains(&pool_info.positions, position_index)) {
abort EPOSITION_NOT_EXIST
};
let position = table::borrow(&pool_info.positions, position_index);
(position.tick_lower_index, position.tick_upper_index)
}
public fun get_position_tick_range<CoinTypeA, CoinTypeB>(
pool_address: address,
position_index: u64
): (I64, I64) acquires Pool {
let pool_info = borrow_global<Pool<CoinTypeA, CoinTypeB>>(pool_address);
get_position_tick_range_by_pool(pool_info, position_index)
}
// rewrite the collect_rewarder function
public fun collect_rewarder<CoinTypeA, CoinTypeB, CoinTypeC>(
account: &signer,
pool_address: address,
position_index: u64,
rewarder_index: u8,
recalculate: bool,
): Coin<CoinTypeC> acquires Pool {
check_position_authority<CoinTypeA, CoinTypeB>(account, pool_address, position_index);
let pool = borrow_global_mut<Pool<CoinTypeA, CoinTypeB>>(pool_address);
assert_status(pool);
update_rewarder(pool);
let position = if (recalculate) {
let (tick_lower, tick_upper) = get_position_tick_range_by_pool<CoinTypeA, CoinTypeB>(pool, position_index);
let rewards_growth_inside = get_reward_in_tick_range(pool, tick_lower, tick_upper);
let position = table::borrow_mut(&mut pool.positions, position_index);
update_position_rewarder(position, rewards_growth_inside);
position
} else {
table::borrow_mut(&mut pool.positions, position_index)
};
......
}19. Dependency git rev should be a commit hash or a tag instead of a branch for reproducibility
Severity: Medium
Status: Fixed
Descriptions: The dependency git rev should be a commit hash or a tag instead of a branch for reproducibility. The branch may be updated in the future, which may cause the build to fail. An example is the frozen version(git commit hash 411cc86b1b8bd1f1ea7a8b9befd97cc3bf104efa) of cetus-clmm can not be compiled with the latest main branch of aptos-core(git commit hash b362344e4b74dc20caad254d356067fcf713353a). While after changing the rev to a previous version(git commit hash e5a0c085143c50dcac711c534e6b4b93d7647c29), it can be compiled successfully.
Code Location: Move.toml
[dependencies.AptosFramework]
git = 'https://github.com/aptos-labs/aptos-core.git'
rev = 'main'
subdir = 'aptos-move/framework/aptos-framework'Suggestion: Use a commit hash or a tag instead of a branch for the dependency git rev.
[dependencies.AptosToken]
git = "https://github.com/aptos-labs/aptos-core.git"
subdir = "aptos-move/framework/aptos-token"
rev = "e5a0c085143c50dcac711c534e6b4b93d7647c29"20. The pool Coin Order Handle
Severity: Medium
Status: Pending
Descriptions: In create_pool<CoinTypeA, CoinTypeB>, a SimpleMap<PoolId, address>will be kept in the Pools. The pool id is a struct of { CoinTypeA, CoinTypeB, tick_spacing }. We can not create a new pool with the same coins and tick_spacing because the seed to generate the pool signer is derived from hash(sorted(CoinTypeA, CoinTypeB), tick_spacing).
There may be PoolId { CoinA, CoinB, TickSpacing0 } and PoolId { CoinB, CoinA, TickSpacing1 } in the Pools at the same time. It might be confusing for the users and inconvenient for the front-end developers in the future.
The assert!( !simple_map::contains_key<PoolId, address>(&pools.data, &pool_id), EPOOL_ALREADY_INITIALIZED ) in this function will never be triggered. If PoolId { CoinA, CoinB, TickSpacing } is already in the Pools, then PoolId { CoinA, CoinB, TickSpacing } and PoolId { CoinB, CoinA, TickSpacing } will both be aborted in account::create_resource_account(&pool_owner_signer, pool_seed); with ERESOURCE_ACCCOUNT_EXISTS.
Commit: 25d115473799a9db777837553bd5e78bf88ca03a
Code Location: sources/factory.move, line 73
public fun create_pool<CoinTypeA, CoinTypeB>(
account: &signer,
tick_spacing: u64,
initialize_price: u128,
uri: String
): address acquires PoolOwner, Pools {
......
// Create pool account
let pool_id = new_pool_id<CoinTypeA, CoinTypeB>(tick_spacing);
let pool_owner = borrow_global<PoolOwner>(@cetus_clmm);
let pool_owner_signer = account::create_signer_with_capability(&pool_owner.signer_capability);
let pool_seed = new_pool_seed<CoinTypeA, CoinTypeB>(tick_spacing);
let pool_seed = bcs::to_bytes<PoolId>(&pool_seed);
let (pool_signer, signer_cap) = account::create_resource_account(&pool_owner_signer, pool_seed);
let pool_address = signer::address_of(&pool_signer);
let pools = borrow_global_mut<Pools>(@cetus_clmm);
pools.index = pools.index + 1;
assert!(
!simple_map::contains_key<PoolId, address>(&pools.data, &pool_id),
EPOOL_ALREADY_INITIALIZED
);
simple_map::add<PoolId, address>(&mut pools.data, pool_id, pool_address);
......
}Suggestion: Force the user to create a pool with coins in order. For example, create_pool<CoinA, CoinB> will succeed while create_pool<CoinB, CoinA> will fail. Adding a coin order assert in create_pool will solve this. And use pool_seed as the key of pools.
Summary of Findings
Our audit team read the documents on https://cetus-1.gitbook.io/cetus-docs/ and reviewed the code of the Cetus Aptos project. We decide to mainly focus on reviewing the code security and normative, then conducted code running tests and business logic security tests on the local test net, and performed a simulation in python which took a deep look at the numeric arithmetic operation. The audit team has been in close contact with the developing team for the past two weeks and discussed these issues together, the development team has addressed most of the issues.
Feedback from Henry — Cofounder of Cetus:
‘ The professionalism and perfectionism that MoveBit showed in the whole auditing process for our system has proven why they can stand out from the market. They gave us more confidence in building safe, robust and competitive products in Move. ’
Feedback from 0xYi — Cofounder of MoveBit:
‘ Cetus is the Pioneer DEX and Concentrated Liquidity Protocol Built on #Aptos and #Sui , and the concentrated liquidity market maker (CLMM) is a next-generation automated market maker. The code quality in Cetus is pretty high and so honored to work with the first CLMM on Aptos Mainnet ’
In general, MoveBit as a security audit team focused on the Move ecosystem, always secure the assets of all users on the Move Ecosystem. The cooperation with Cetus to audit the centralized liquidity agreement on Aptos blockchain will not only assist Cetus to continuously meet the comprehensive needs of traders, liquidity providers, and growing DeFi users; but also It can more effectively accelerate the overall development of the Aptos blockchain, so as to realize the common vision of the Aptos blockchain — Become the mainstream blockchain in the future.
About Cetus
Cetus is a pioneer DEX and concentrated liquidity protocol focusing on Move-based ecosystems like Aptos and Sui. It’ll work as a crucial part of the ecosystem infrastructure to satisfy the comprehensive needs of traders, LPs, upper applications and an increasing DeFi population.
Cetus Social Media Platforms:
Website | Twitter | Discord | Medium | Telegram Community | Telegram Annoucement
About MoveBit
MoveBit is a blockchain security company focused on the Move Ecosystem by pioneering the use of cutting-edge Formal Verification. The team consists of security professionals from academia and enterprise with 10 years of security experience. they were one of the earliest contributors to the Move ecosystem, working with Move developers to set the standard for secure Move applications and make the Move ecosystem the most secure Web3 destination
MoveBit Social Media Platforms:
Official Website | Twitter | Medium | GitHub | Discord
