2024-03-21 liquidity migration audit report

Security Audit Report

Neutron 2024 Q1 - Liquidity Migration

Authors: Ivan Gavran, Aleksandar Ignjatijevic

Table of Contents Audit Overview ............................................................................................................ 1 Scope 1 Conclusion 1 Audit Dashboard ......................................................................................................... 2 Target Summary 2 Engagement Summary 2 Severity Summary 2 Findings ....................................................................................................................... 3 An attacker can write to the lockdrop's contract memory at will 4 Reward amounts in lockdrop-pcl contract are miscalculated in case of time-extended migration 7 The migration may temporarily fail for some users due to manipulation of pool ratios 9 Lockdrop-PCL is relying on an unspecified behavior of the Incentive's PendingRewards query 11 Miscellaneous code concerns 13 Miscellaneous security concerns 16 Testing Issues 17 Appendix: Model-based Testing ............................................................................... 28 Model state 28 Initialization action: users are given different number of tokens initially 28 Step Action 29 Test property definition 30 Appendix: Vulnerability Classification ..................................................................... 31 Impact Score 31 Exploitability Score 31 Severity Score 32 Appendix: Calculation of lockdrop pool rewards .................................................... 34 Disclaimer.................................................................................................................. 35 © 2024 Informal Systems Neutron 2024 Q1 - Liquidity Migration

Audit Overview Scope In March 2024, Informal Systems conducted a security audit for Neutron. The audit focused on liquidity migration of lockdrop and vesting contracts from XYK pools to PCL pools. (The reserve contract’s migration was audited in a previous audit.) The audit consisted of code review and designing an extended set of end-to-end test cases, built on top of the existing testing infrastructure. The audit was performed from February 28, 2024 to March 21, 2024 by the following personnel: • Ivan Gavran • Aleksandar Ignjatijevic

Relevant Code Commits The scope of the audit were the following repositories: • neutron-tge-contracts at e86b087 • neutron-dao at ef1b14f

The provided set of end-to-end tests was neutron-integration-tests, branch feat/migrate-to-pcl .

Conclusion We performed a thorough code review of the liquidity migration and found it to be elegantly designed, relying on existing code primitives of withdrawing and depositing funds. We also augmented the existing end-to-end testsuite with model-based test scenarios, which varied different interleavings of steps within migration. (The testing methodology is described in Appendix: Model-based Testing .) Overall, we found two high-impact issues in the code, and a number of lower risk ones. The dev team promptly addressed the findings. We reviewed the fixes and found them to be properly address the reported issues.

Audit Dashboard Target Summary • Type: Protocol and Implementation • Platform: CosmWasm

Engagement Summary • Dates: 28.2.2024. - 21.3.2024. • Method: Manual code review, protocol analysis, model-based fuzzing

Severity Summary Finding Severity #

Critical 0

High 2

Medium 0

Low 2

Informational 2

Total 6

The table above contains only the findings for which we provided both the problematic scenario and the root cause analysis. On top of that, we provided 5 more problematic scenarios obtained by model-based testing, for which the dev team performed the root cause analysis independently. (For details, see Testing Issues.)

Findings Title Type Severity Status

An attacker can write to the lockdrop's Implementation 3 High Resolved contract memory at will

Reward amounts in lockdrop-pcl contract Implementation 3 High Resolved are miscalculated in case of time-extended migration

The migration may temporarily fail for Protocol 1 Low Risk Accepted some users due to manipulation of pool ratios

Lockdrop-PCL is relying on an unspecified Implementation 1 Low Resolved behavior of the Incentive's PendingRewards query

Miscellaneous code concerns Practice 0 Informational Risk Accepted

Miscellaneous security concerns Practice 0 Informational Resolved

An attacker can write to the lockdrop's contract memory at will Type Implementation

Severity 3 High

Impact 3 High

Exploitability 2 Medium

Status Resolved

Involved artifacts • neutron-tge-contracts/contracts/lockdrop/src/contract.rs::handle_migrate_liquidity_to_pcl_pools • neutron-tge-contracts/contracts/lockdrop/src/contract.rs::handle_claim_rewards_and_unlock_for_lockup • neutron-tge-contracts/contracts/lockdrop-pcl/src/ contract.rs::handle_claim_rewards_and_unlock_for_lockup • https://github.com/informalsystems/neutron-tge-contracts-audit-internal/blob/feat/pcl-lp-migration/ contracts/lockdrop/src/testing.rs#L10 .

Description The ExecuteMsg::MigrateLiquidityToPCLPools takes a user_address_raw optional string parameter. This string denotes the address of the user whose liquidity is supposed to be migrated.

The address is validated first, turning user_address_raw into user_address of type Addr , and then the corresponding user is loaded with the following code-snippet:

let mut user_info = USER_INFO .may_load(deps.storage, &user_address)? .unwrap_or_default();

The problem with this snippet is that if there is no user under user_address , it will return a default UserInfo structure. The default structure is:

UserInfo { pub total_ntrn_rewards: 0 pub ntrn_transferred: false, pub lockup_positions_index: 0, }

Because total_ntrn_rewards equals 0, the check that follows will pass:

if user_info.total_ntrn_rewards == Uint128::zero() { user_info.total_ntrn_rewards = update_user_lockup_positions_and_calc_rewards( deps.branch(),

&config, &state, &user_address, )?; USER_INFO.save(deps.storage, &user_address, &user_info)?; }

The non-existent user will first enter the update_user_lockup_positions_and_calc_rewards function, not causing much trouble there because there are no lockups associated with that user, and most of the function body will turn into no-ops.

After returning from the function, the user will be saved into the USER_INFO map.

After that point, the function iterates over lockups (no-op in this case) to create messages for the migration to continue. Since no messages will be generated, this message handler will simply return Ok(Response::default()).

Problem Scenarios This behavior enables an attacker to run a script sending batches of ExecuteMsg::MigrateLiquidityToPCLPools messages with arbitrary user_address_raw fields (up to the validation). This will result in validators' memory filling up and bloating the state of the contract, as a consequence making the sync/state export/state import extremely slow, and potentially resulting in out-of-memory restarts of individual validators. • Note A: This attack is expensive due to gas cost, and can only be carried out over a number of blocks (or otherwise gas limit would be reached). • Note B: This attack can be performed at any point, given that the migration entrypoints remain active after the migration is performed. • Note C: The lockdrop::handle_claim_rewards_and_unlock_for_lockup and lockdrop- pcl::handle_claim_rewards_and_unlock_for_lockup contain similar code snippets. There, though, the attack will not go through because of the call to compatible_load, which would result in an error and revert the whole transaction. We recommend nonetheless to add an explicit check there and return an error early in the function. The test illustrating the attack can be found at https://github.com/informalsystems/neutron-tge-contracts-audit- internal/blob/feat/pcl-lp-migration/contracts/lockdrop/src/testing.rs#L10 . We attach a (failing) test illustrating the problem:

fn handle_migrate_liquidity_to_pcl_pools_test(){

let env = mock_env(); let user_addr = Some("newaddr".to_string()); let info = mock_info("addr0000", &[]); let mut deps = mock_dependencies(); let owner = Addr::unchecked("owner"); let token_info_manager = Addr::unchecked("token_info_manager");

let msg = InstantiateMsg { owner: Some(owner.to_string()), token_info_manager: token_info_manager.to_string(), init_timestamp: env.block.time.seconds(),

lock_window: 10_000_000, withdrawal_window: 500_000, min_lock_duration: 1u64, max_lock_duration: 52u64, max_positions_per_user: 14, credits_contract: "credit_contract".to_string(), lockup_rewards_info: vec![LockupRewardsInfo { duration: 1, coefficient: Decimal256::zero(), }], auction_contract: "auction_contract".to_string(), };

instantiate(deps.as_mut(), env.clone(), info.clone(), msg).unwrap(); let user_info_keys = USER_INFO .keys(deps.as_mut().storage, None, None, cosmwasm_std::Order::Ascending) .collect::<Result<Vec<Addr>, StdError>>() .expect("error"); dbg!(user_info_keys.len()); assert!(user_info_keys.len() == 0);

let _r = handle_migrate_liquidity_to_pcl_pools(deps.as_mut(), info, env, user_addr).unwrap();

let user_info_keys = USER_INFO .keys(deps.as_mut().storage, None, None, cosmwasm_std::Order::Ascending) .collect::<Result<Vec<Addr>, StdError>>() .expect("error"); dbg!(user_info_keys.len()); assert!(user_info_keys.len() == 0);

}

Recommendation We recommend checking for the address’s existence as early as possible, ideally at the very beginning of the handle_migrate_liquidity_to_pcl_pools .

Status Successfully resolved in PR#84.

Reward amounts in lockdrop-pcl contract are miscalculated in case of time-extended migration Type Implementation

Severity 3 High

Impact 3 High

Exploitability 2 Medium

Status Resolved

Involved artifacts • neutron-tge-contracts/lockdrop-pcl/src/contract.rs • astroport-core/contracts/tokenomics/incentives/src/utils.rs

Description The basic idea in calculating the amount of rewards belonging to each lockup position is the following: every time there is a claim from the pool, the pool_info ’s field incentives_rewards_per_share is updated by adding to it received_amount / total_lp_balance_deposited . (More details on how the calculation is performed can be found in Appendix: Calculation of lockdrop pool rewards.) This ensures that each lockup that existed in between two claims gets “assigned” its share of reward. There is one exception, though: each time new liquidity is provided to the pool, the pool also sends pending rewards to the lockdrop-pcl contract, but pool_info.incentives_rewards_per_share does not get updated.

Problem Scenarios Assume two contracts, A and B , being migrated from the XYK to the PCL pool. Assume also that A is migrated first and then there is a long break before B is migrated. Furthermore, there are no claims from the lockdrop- pcl contract before B gets migrated.

For simplicity (but without loss of generality), assume that at the moment that A is being migrated, there is 0 ASTRO in lockdrop-pcl and 0 pending rewards. Let’s consider the following time points:

A gets migrated: lockdrop-pcl ASTRO amount: 0; pending rewards: 0
Some time passes in which A is the only lockup in the contract. lockdrop-pcl ASTRO amount: 0; pending rewards: 100
B gets migrated. Now the pool sends all the pending rewards to lockdrop-pcl . lockdrop-pcl ASTRO amount: 100; pending rewards: 0
Some time passes with both A and B in the pool: lockdrop-pcl ASTRO amount: 100; pending rewards: 20

A claims its rewards. According to the formula, incentives_rewards_per_share will get calculated using the received_amount = current_balance - pervious_balance . The problem is that previous_balance is the balance just before the claim, thus: 100. Therefore, received_amount = 20 . Now, A will receive its rewards using incentives_per_share that completely disregards the rewards accrued while it was alone in the pool. This amount will stay in the lockdrop-pcl contract.

NOTE: The consequences of the problem presented above are going to be minimized if all the contracts are executed in a quick succession one after another, followed by a claim. Alternatively, they will be maximized for longer periods between the first migration and the first claim from lockdrop-pcl .

Recommendation Update the state, similarly to how it is done in update_pool_on_dual_rewards_claim, after each deposit. Make sure to use the amount of LP tokens before the deposit happened in the calculation.

Status Successfully resolved in PR#85.

The migration may temporarily fail for some users due to manipulation of pool ratios Type Protocol

Severity 1 Low

Impact 1 Low

Exploitability 1 Low

Status Risk Accepted

Description In this finding, we are describing a scenario in the migration of certain users' funds will not succeed due to imbalanced ratios of liquidity pools. The failure will have no material consequences other than slowing down the migration process and requiring a retry mechanism for the users whose liquidity was not successfully migrated.

Assume an attacker whose intention is to disrupt the ratio within the xyk pool before or during the migration. Symmetrically, the attacker may consider disrupting the ratio of the PCL pool. Such an attacker should be prepared to lose some funds while performing the attack.

Let us focus our attention on disrupting the xyk pool. The attacker may do so with the least amount of liquidity around the end of the migration process (for the PCL , the attacker would likely act at the very beginning of the process), when the pool is shallow. Principally, the attack is possible because of the high percentage of liquidity in the pools coming from Neutron contracts, thus making sure that the pools will eventually be fairly shallow.

Disruption of the xyk pool ratio will result in trying to provide_liquidity with assets in the disrupted ratio to the PCL pool. This will result in the transaction failing, because of the slippage tolerance on the PCL pool side. Depending on the attacker’s next actions, other users may be migrated successfully or the fail scenario will be the same for them.

This will keep happening until either the malicious user decides to sell NTRN back to the pool, or until the market makers have balanced the pool.

Problem Scenarios The attacker buys significant amount of NTRN from the pool, using its pair token, in the same block when the migration of the user u is happening. That would disrupt the pool ratio between those two tokens and now in the pool will be very low amount of NTRN and a very large amount of its pair token.

In the migration process, user u will get NTRN s and paired tokens in the (disbalanced) proportion of the pool.

From here the problem branches into two subscenarios: A. new PCL pool is fresh and has no funds in it B. PCL pool has already correct ratio in itself

Subscenario A: There is a fresh PCL pool with no funds in it. This scenario is possible only if the pool ratio has been disrupted right before the migration of the first user u , and the pool was empty before that.

User u will be migrated to the empty PCL pool, establishing the wrong ratio as the pool ratio. All subsequent users' migration will fail, due to slippage tolerance on the PCL pool side. This will keep happening until the market makers correct the PCL pool ratio. If the attacker is able to beat the market makers and sell whole amount of NTRN back to the xyk pool, that would make his losses only the fee price.

Subscenario B: PCL pool already has the correct ratio in itself

User u will not be migrated, due to slippage tolerance on the PCL pool side. All subsequent users will meet the same faith until the market makers have fixed the xyk pool ratio. Same as in Subscenario A, if the attacker is able to beat the market makers and sell whole amount of NTRN back to the xyk pool, that would make his losses only the fee price.

Furthermore, a symmetrical attack may be performed on the PCL pool when it is in its shallow state.

Recommendation Having insight into users whose migration has failed and having in mind a retry mechanism for said users, renders the attack harmless in the long run. Furthermore, once the xyk pool becomes very shallow, there may be little incentive there for market makers: thus, it would be a good idea to have an own market maker constantly working between the two pools.

Lockdrop-PCL is relying on an unspecified behavior of the Incentive's PendingRewards query Type Implementation

Severity 1 Low

Impact 1 Low

Exploitability 1 Low

Status Resolved

Involved artifacts • neutron-tge-contracts/contracts/lockdrop-pcl/src/contract.rs

Description When handling the ExecuteMsg::ClaimRewardsAndOptionallyUnlock message, lockdrop-pcl first claims all the pending rewards, then updates the relevant state variables, and finally sends the rewards to the user in callback_withdraw_user_rewards_for_lockup_optional_withdraw .

In that function, it is invoking the IncentivesQueryMsg::PendingRewards query: since all the rewards have been recently claimed, there will be no pending rewards. The logic that follows does not need pending amounts, but only the corresponding AssetInfo (it relies on the updated state variable pool_info.incentives_rewards_per_share ). Conveniently, the invoked query will return a vector of Asset s that will contain all the assets with 0 amounts.

This behavior is indeed exhibited by the query, but it is not the only possible behavior given the specification of the query: “PendingToken returns the amount of rewards that can be claimed by an account that deposited a specific LP token in a generator” Another possible return value in the situation with no pending rewards would be an empty vector.

Problem Scenarios If Astroport were to change the return format of the query, there would likely be no warning of it since it is not a breaking change. Then, the logic of the withdraw callback would break, since the loop going over all pending rewards (in reality: over all assets that may qualify for rewards) would turn into a no-op.

Recommendation We recommend sending a list of assets to check as a parameter to the callback function.

Status Successfully resolved in PR#87.

Miscellaneous code concerns Type Practice

Severity 0 Informational

Impact 1 Low

Exploitability 0 None

Status Risk Accepted

We list here various code-related concerns that do not pose security threats, but addressing them may make the codebase more robust, readable, and maintainable. • README.md from lockdrop-vault-pcl-pools has some misleading information. It is supposed to collect voting power from Lockdrop-pcl contract (but instead it claims to be collecting from the Lockdrop contract). • There are a couple of misleading comments in the callback_transfer_all_rewards_before_migration function (here, here, and here). They suggest that rewards are being claimed in the function, when in reality they are only distributed. •

Excerpt (19994 of 55707 characters). Read the whole page on informalsystems/audits ↗

← Back to Documentation