2023-11-05 Audit Report - Neutron Smart Contracts - Overrule + Liquidity Migration

Security Audit Report

Neutron Smart Contracts: Overrule + Liquidity Migration

Authors: Ivan Gavran, Andrey Kuprianov, Nikola Jovicevic

Table of Contents Audit overview............................................................................................................. 1 The Project 1 Conclusions 1 Audit dashboard.......................................................................................................... 2 Target Summary 2 Engagement Summary 2 Severity Summary 2 Identified Threats and the Audit Plan ........................................................................ 3 Findings ....................................................................................................................... 4 LP Vesting contract may get permanently stuck in the migration process 5 Reliance on contract token balance to provide liquidity may permanently disable Lockdrop contract migration 9 Absence of safeguards in voting power calculations may lead to voting overturn 12 Risk of instantiating malicious contracts 19 Overly permissive handling of a pre-proposal error 21 Duplicate contract names 23 Non-intended behaviour upon failure to instantiate a pre-proposal module 24 Documentation: quorum in overrule contracts 26 Code Improvements 27 Premature state update in MigrateLiquidity message execution 29 Explicit Check of the Assumptions 31 Appendix: Vulnerability Classification ..................................................................... 33 Impact Score 33 Exploitability Score 33 Severity Score 34 Disclaimer.................................................................................................................. 36 © 2023 Informal Systems Neutron Smart Contracts: Overrule + Liquidity Migration

Audit overview The Project In September 2023, Informal Systems has conducted a security audit for Neutron. The audit covered two areas:

Governance smart contracts allowing the main Neutron DAO to overrule passed proposals of subDAOs. The audited repository was neutron-dao at the commit hash 1cd5d88, focusing on the following contracts: a. neutron-dao/contracts/dao/pre-propose/cwd-pre-propose-single-overrule b. neutron-dao/contracts/subdaos/pre-propose/cwd-subdao-pre-propose- single c. neutron-dao/contracts/subdaos/cwd-subdao-timelock-single d. neutron-dao/contracts/subdaos/pre-propose/cwd-security-subdao-pre- propose
The update of lockdrop, reserve, and vesting-lp smart contracts that enables transferring funds from XYK pools to concentrated liquidity (CL) pools. This implied auditing the neutron-dao repository at the commit hash 1cd5d88, and the neutron-tge-contracts repository at the commit hash 410d560. The contracts of particular interests for the audit were a. neutron-dao/contracts/dao/voting/neutron-voting-registry b. neutron-dao/contracts/dao/voting/lockdrop-vault-for-cl-pools c. neutron-dao/contracts/dao/voting/vesting-lp-vault-for-cl-pools d. neutron-dao/contracts/tokenomics/reserve e. neutron-tge-contracts/contracts/lockdrop f. neutron-tge-contracts/contracts/vesting-lp

The audit was performed from September 4, 2023 through September 29, 2023, by the following personnel: • Ivan Gavran • Andrey Kuprianov • Nikola Jovicevic

Conclusions We performed a thorough review of the contracts, as well as the deployment workflow, and found it to be implemented carefully. Nonetheless, some subtle mistakes were found. In our audit, we report the total of 11 findings: two of them of the critical severity, and one of the high severity.

Audit dashboard Target Summary • Type: Protocol and Implementation • Platform: CosmWasm • Artifacts: neutron-dao, neutron-tge-contracts

Engagement Summary • Dates: 04.09.2023 -- 29.09.2023 • Method: Manual code review, protocol analysis

Severity Summary Finding Severity #

Critical 2

High 1

Medium 0

Low 2

Informational 6

Total 11

Identified Threats and the Audit Plan When analyzing both parts of the audit, governance overrule contracts and liquidity migration contracts, we focused first on understanding the protocol, looking for potential errors in the protocol, and finally inspecting the code to find implementation errors. During our analysis, we inspected in particular the following general threats:

Crossing Trust Boundaries: any input coming from an independent (and thus potentially malicious) source must not be accepted unconditionally.
Inter-Contract Interactions: whether the contracts employed during execution respect the expected contracts, and whether they can be influenced with malicious intents.
Error Handling: every potential failure in the sequence of the messages needs to be handled appropriately.
Mathematical Computations: whether all computations are performed correctly, and if there are problems due to e.g. incorrect datatype usage or rounding
Access Control: entry points of a contract should contain an access check (unless being permissionless is an intended setup).
Migration Setup: best practices when migrating contracts need to be followed.
State Variables: each state variable that gets changed needs to be eventually updated in the storage.
CosmWasm Message Transfer: whether the used patterns of messages and submessages correspond to the desired effects, e.g. whether message flows are uninterrupted, or all submessage results are processed correctly.

Specific to the analyzed contracts, we inspected the following questions:

Are the checks that are in place before creating an overrule proposal sufficient, and are they implemented correctly.
Are there any flaws resulting in the violation of the overrule contract invariant: If a subDAO (other than security subDAO) votes in a proposal, it will get timelocked and the main dao will get a chance to overrule it. If it votes to overrule, it will not be executed. Otherwise, it will be executed. A security subDAO won’t be executed, but can only propose Pause and RemoveSchedule messages.
Is it possible for the Security subDAO to use its no-timelock power to propose and execute messages other than Pause and RemoveSchedule .
Is it possible that a migration status state gets stuck mid-migration and cannot be changed.
Inspect the querying of voting power and check numerical calculations.
Could the restriction on the maximum slippage block the migration.

In the audit process, we inspected the listed threats, resulting in the findings presented in the next section.

Findings Title Type Severity Status

LP Vesting contract may get permanently IMPLEMENTATION 4 CRITICAL RESOLVED stuck in the migration process

Reliance on contract token balance to IMPLEMENTATION 4 CRITICAL RESOLVED provide liquidity may permanently disable Lockdrop contract migration

Absence of safeguards in voting power PROTOCOL 3 HIGH RISK ACCEPTED calculations may lead to voting overturn IMPLEMENTATION

Risk of instantiating malicious contracts IMPLEMENTATION 1 LOW RISK ACCEPTED

Overly permissive handling of a pre- PROTOCOL 1 LOW RISK ACCEPTED proposal error

Duplicate contract names IMPLEMENTATION 0 INFORMATIONAL RESOLVED

Non-intended behaviour upon failure to IMPLEMENTATION 0 INFORMATIONAL RESOLVED instantiate a pre-proposal module

Documentation: quorum in overrule DOCUMENTATION 0 INFORMATIONAL RESOLVED contracts

Code Improvements PRACTICE 0 INFORMATIONAL RESOLVED

Premature state update in MigrateLiquidity IMPLEMENTATION 0 INFORMATIONAL ACKNOWLEDGED message execution

Explicit Check of the Assumptions PRACTICE 0 INFORMATIONAL RESOLVED

LP Vesting contract may get permanently stuck in the migration process Title LP Vesting contract may get permanently stuck in the migration process

Project Neutron Q3 2023

Type IMPLEMENTATION

Severity 4 CRITICAL

Impact 3 HIGH

Exploitability 3 HIGH

Threats Broken state machine logic

Author(s) Andrey Kuprianov

Status RESOLVED

Link(s) https://github.com/neutron-org/neutron-tge-contracts/pull/60 https://github.com/neutron-org/neutron-integration-tests/pull/218

Involved artifacts • neutron-tge-contracts at branch neutron_audit_informal_04_09_2023 : • vesting-lp contract • vesting-lp-base handlers

Description The vesting-lp contract performs its actions via delegating message execution, queries, and migration to the vesting-lp-base handlers.

Execution of all normal contract messages is preconditioned that the migration process is completed as follows:

pub fn execute( deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg, ) -> Result<Response, ContractError> { let migration_state = MIGRATION_STATUS.may_load(deps.storage)?;

if migration_state.unwrap_or(MigrationState::Completed) != MigrationState::Completed { match msg { ExecuteMsg::MigrateLiquidity { slippage_tolerance: _, } => {} ExecuteMsg::Callback(..) => {} _ => return Err(ContractError::MigrationIncomplete {}), } } match msg { ...

This means that before the migration is completed, only the MigrateLiquidity message, as well as migration callbacks can execute.

MigrateLiquidity is processed by function execute_migrate_liquidity(), which looks like this:

fn execute_migrate_liquidity( deps: DepsMut, env: Env, slippage_tolerance: Option<Decimal>, ) -> Result<Response, ContractError> { let migration_state: MigrationState = MIGRATION_STATUS.load(deps.storage)?; if migration_state == MigrationState::Completed { return Err(ContractError::MigrationComplete {}); } let migration_config: XykToClMigrationConfig = XYK_TO_CL_MIGRATION_CONFIG.load(deps.storage)?;

let vesting_infos = read_vesting_infos( deps.as_ref(), migration_config.last_processed_user, Some(migration_config.batch_size), None, )?;

let vesting_accounts: Vec<_> = vesting_infos .into_iter() .map(|(address, info)| VestingAccountResponse { address, info }) .collect();

if vesting_accounts.is_empty() { MIGRATION_STATUS.save(deps.storage, &MigrationState::Completed)?; } let mut resp = Response::default();

// get pairs LP token addresses let pair_info: PairInfo = deps .querier .query_wasm_smart(migration_config.xyk_pair.clone(), &PairQueryMsg::Pair {})?;

// query max available amounts to be withdrawn from pool let max_available_amount = { let resp: BalanceResponse = deps.querier.query_wasm_smart( pair_info.liquidity_token.clone(), &Cw20QueryMsg::Balance { address: env.contract.address.to_string(), }, )?; resp.balance };

if max_available_amount.is_zero() { return Ok(resp); }

for user in vesting_accounts.into_iter() { ... }

Ok(resp) }

The migration state, which is initialized to MigrationState::Started in function migrate() is set to MigrationState::Completed in the code above only when no vesting accounts remain in the contract state to be processed, as determined by the variable migration_config.last_processed_user . This variable is, in turn, updated only in the last callback called in the chain of callbacks, namely post_migration_vesting_reschedule_callback().

In order to not exceed the gas limit, function execute_migrate_liquidity requests and processes vesting accounts in batches of size migration_config.batch_size ; this means that this function may need to execute multiple times. The problem lies in how the above two functionalities, namely batched processing and migration state machine, interact. In particular, the code above is executed at the beginning of each batch, and contains the following fragment:

if max_available_amount.is_zero() { return Ok(resp); }

The above fragment queries the current balance of liquidity tokens, and terminates the function early if the balance reaches zero, assuming that nothing left to do. The problem is that while the balance reaches zero, there might be still vesting accounts remaining, for which all tokens have been released. This means that the code that sets migration status to MigrationState::Completed never gets executed, and the contract gets stuck in the MigrationState::Started state.

Problem Scenarios It may happen that at the moment of performing migration, the combination of the current set of vesting accounts, and the migration batch size will be such that the remainder of the set will have all funds released, but still some vesting accounts left; thus leading to the situation described above. The result will be the contract being permanently locked in the MigrationState::Started state, unable to execute any further messages.

Recommendation Make the conditions of "no vesting accounts left" and "no available funds left" equal in power, and set the migration state to MigrationState::Completed in both cases.

Reliance on contract token balance to provide liquidity may permanently disable Lockdrop contract migration Title Reliance on contract token balance to provide liquidity may permanently disable Lockdrop contract migration

Project Neutron Q3 2023

Type IMPLEMENTATION

Severity 4 CRITICAL

Impact 3 HIGH

Exploitability 3 HIGH

Threats Influence by dynamic environment

Author(s) Andrey Kuprianov

Status RESOLVED

Link(s) https://github.com/neutron-org/neutron-tge-contracts/pull/58 https://github.com/neutron-org/neutron-integration-tests/pull/210

Involved artifacts • neutron-tge-contracts at branch neutron_audit_informal_04_09_2023 : • lockdrop contract

Description In the lockdrop contract, in function migrate_pair_step_2(), the following fragment is present:

let token_denom = new_pool_info .assets .iter() .find_map(|x| match &x.info { AssetInfo::NativeToken { denom } if denom != UNTRN_DENOM => Some(denom.clone()), _ => None, }) .ok_or_else(|| StdError::generic_err("No second leg of pair found"))?; attrs.push(attr("token_denom", token_denom.clone())); //calculate amount of token (ATOM|USDC) claimed from pool

let token_balance = deps .querier .query_balance(&env.contract.address, token_denom.as_str())? .amount; attrs.push(attr("token_balance", token_balance.to_string()));

//construct message to send NTRN and (ATOM|USDC) to new pool let base = Asset { amount: ntrn_to_new_pool, info: AssetInfo::NativeToken { denom: UNTRN_DENOM.to_string(), }, }; let other = Asset { amount: token_balance, info: AssetInfo::NativeToken { denom: token_denom.to_string(), }, }; let mut funds = vec![ Coin { denom: UNTRN_DENOM.to_string(), amount: ntrn_to_new_pool, }, Coin { denom: token_denom, amount: token_balance, }, ]; funds.sort_by(|a, b| a.denom.cmp(&b.denom)); msgs.push(CosmosMsg::Wasm(WasmMsg::Execute { contract_addr: new_pool_addr, funds, msg: to_binary(&astroport::pair::ExecuteMsg::ProvideLiquidity { assets: vec![base, other], slippage_tolerance, auto_stake: None, receiver: None, })?, }));

In the above fragment, the current token balance of the contract is queried, with the intention to "calculate amount of token (ATOM|USDC) claimed from pool". The problem is, unlike NTRN amount, which is calculated as the difference between the previous and the current token balances, for the ATOM/USDC claimed amount the current token balance is used, and then passed directly as one of the asset amounts in the message astroport::pair::ExecuteMsg::ProvideLiquidity .

Function migrate_pair_step_2() is part of a sequence of calls, originating from processing of the message ExecuteMsg::MigrateFromXykToCl , which is permissionless. Anyone can call this function, providing additional funds with it, and this altering the contract token balance, which will lead to the wrong liquidity provided to the Astroport pair. As slippage tolerance is checked in this function, the additional provided liquidity may exceed the allowed slippage tolerance, thus making the migration to fail permanently.

Problem Scenarios The following scenario is possible:

The new variant of the contract is deployed, and the migration starts
An attacker submits the message ExecuteMsg::MigrateFromXykToCl (which is permissionless), and sends additional funds with it.
The function fails, due to exceeding the slippage tolerance.
The contract is permanently locked, as migration will always fail from now on, and no other functions are allowed to execute (as migration is in progress).

Recommendation We recommend to use the same scheme for getting the claimed amount of ATOM/USDC tokens as for NTRN tokens, namely as the difference in the contract token balance before and after withdrawing from the XYK pool.

Absence of safeguards in voting power calculations may lead to voting overturn Title Absence of safeguards in voting power calculations may lead to voting overturn

Project Neutron Q3 2023

Type PROTOCOL IMPLEMENTATION

Severity 3 HIGH

Impact 3 HIGH

Exploitability 2 MEDIUM

Threats Crossing trust boundaries Influence by dynamic environment

Author(s) Andrey Kuprianov

Status RISK ACCEPTED

Link(s)

Involved artifacts • neutron-dao at branch neutron_audit_informal_04_09_2023 : • neutron-voting-registry • Neutron vaults: • neutron-vault • credits-vault • investors-vesting-vault • lockdrop-vault • lockdrop-vault-for-cl-pools • vesting-lp-vault • vesting-lp-vault-for-cl-pools

Description Neutron voting registry plays a central role in the Neutron system. Citing from Neutron documentation: Instead of a single voting power module, Neutron DAO core contract interacts with the Voting Power Registry contract that keeps track of multiple Voting Vaults. A voting vault is a smart contract that implements the DAO DAO voting module interface, namely, it is capable of: Providing the total voting power at a given height,Providing the voting power of an address at a given height.

The overall voting power of a given address is a sum of the voting powers that the address has in all of the registered voting vaults. More specifically, Neutron voting registry as well as all voting vaults it interacts with implement queries VotingPowerAtHeight and TotalPowerAtHeight , illustrated below on the example of voting registry:

QueryMsg::VotingPowerAtHeight { address, height } => { to_binary(&query_voting_power_at_height(deps, env, address, height)?) } QueryMsg::TotalPowerAtHeight { height } => { to_binary(&query_total_power_at_height(deps, env, height)?) }

The implementation of query_voting_power_at_height iterates over all stored vaults, queries each of them, and sums up the results as follows:

let mut resp = VotingPowerAtHeightResponse { power: Default::default(), height, }; for vault in vaults { if let Some(vault_state) = VAULT_STATES.may_load_at_height(deps.storage, vault.clone(), height)? { if vault_state == VotingVaultState::Active { let vp_in_vault: VotingPowerAtHeightResponse = deps.querier.query_wasm_smart( vault,

Excerpt (19974 of 56160 characters). Read the whole page on informalsystems/audits ↗

← Back to Documentation