Astroport.fi Astral Assembly - Astroport.fi

Prepared by:

HALBORN

Last Updated 04/26/2024

Date of Engagement by: March 7th, 2022 - March 18th, 2022

Summary

100% of all REPORTED Findings have been addressed

All findings

Critical

High

Medium

Low

Informational

1. Introduction
2. Audit summary
3. Test approach & methodology
4. Scope
5. Risk methodology
6. Scope
7. Assessment summary & findings overview
8. Findings & Tech Details

8.1 Lack of proposal sanitization could harm users
8.2 Inconsistent xastro block height selection
8.3 No minimum threshold for effective delay period
8.4 Unchecked arithmetic

9. Automated Testing

1. INTRODUCTION

Astroport.fi engaged Halborn to conduct a security audit on their smart contracts beginning on March 7th, 2022 and ending on March 18th, 2022. The security assessment was scoped to the smart contracts provided to the Halborn team.

2. AUDIT SUMMARY

The team at Halborn was provided two weeks for the engagement and assigned two full-time security engineers to audit the security of the smart contract. The security engineers are a blockchain and smart-contract security experts with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.

The purpose of this audit is to:

Ensure that smart contract functions operate as intended
Identify potential security issues with the smart contracts

In summary, Halborn identified some improvements to reduce the likelihood and impact of multiple risks, which has been mostly addressed by Astroport.fi. The main ones are the following:

Sanitize proposal information that will be displayed on the web frontend.
Select a consistent block height when requesting token information to avoid potential quorum bypasses.
Enforce thresholds on proposal delay periods.

3. TEST APPROACH & METHODOLOGY

Halborn performed a combination of manual review of the code and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of the smart contract audit. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of smart contracts and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the audit:

Research into architecture, purpose, and use of the platform.
Manual code read and walkthrough.
Manual assessment of use and safety for the critical Rust variables and functions in scope to identify any contracts logic related vulnerability.
Fuzz testing (Halborn custom fuzzing tool)
Checking the test coverage (cargo tarpaulin)
Scanning of Rust files for vulnerabilities (cargo audit)

4. SCOPE

Code repository: astroport-governance

\begin{enumerate} \item CosmWasm Smart Contracts \begin{enumerate} \item Commit ID: \href{https://github.com/astroport-fi/astroport-governance/tree/7918a7c9d1b93a224e91d955da6e360456df8f81}{7918a7c9d1b93a224e91d955da6e360456df8f81} \item Contract within scope: \begin{enumerate} \item assembly \end{enumerate} \end{enumerate} \end{enumerate}

Out-of-scope: External libraries and financial related attacks.

5. RISK METHODOLOGY

Vulnerabilities or issues observed by Halborn are ranked based on the risk assessment methodology by measuring the LIKELIHOOD of a security incident and the IMPACT should an incident occur. This framework works for communicating the characteristics and impacts of technology vulnerabilities. The quantitative model ensures repeatable and accurate measurement while enabling users to see the underlying vulnerability characteristics that were used to generate the Risk scores. For every vulnerability, a risk level will be calculated on a scale of 5 to 1 with 5 being the highest likelihood or impact.

RISK SCALE - LIKELIHOOD

5 - Almost certain an incident will occur.
4 - High probability of an incident occurring.
3 - Potential of a security incident in the long term.
2 - Low probability of an incident occurring.
1 - Very unlikely issue will cause an incident.

RISK SCALE - IMPACT

5 - May cause devastating and unrecoverable impact or loss.
4 - May cause a significant level of impact or loss.
3 - May cause a partial impact or loss to many.
2 - May cause temporary impact or loss.
1 - May cause minimal or un-noticeable impact.

The risk level is then calculated using a sum of these two values, creating a value of 10 to 1 with 10 being the highest level of security risk.

Critical

High

Medium

Low

Informational

10 - CRITICAL
9 - 8 - HIGH
7 - 6 - MEDIUM
5 - 4 - LOW
3 - 1 - VERY LOW AND INFORMATIONAL

6. SCOPE

Out-of-Scope: New features/implementations after the remediation commit IDs.

7. Assessment Summary & Findings Overview

Critical

High

Medium

Low

Informational

Impact x Likelihood

Likelihood

Impact

HAL-03

HAL-01

HAL-02

HAL-04

Security analysis	Risk level	Remediation Date
LACK OF PROPOSAL SANITIZATION COULD HARM USERS	Medium	Partially Solved
INCONSISTENT XASTRO BLOCK HEIGHT SELECTION	Medium	Not Applicable
NO MINIMUM THRESHOLD FOR EFFECTIVE DELAY PERIOD	Low	Solved - 03/25/2022
UNCHECKED ARITHMETIC	Informational	Solved - 03/25/2022

8. Findings & Tech Details

8.1 LACK OF PROPOSAL SANITIZATION COULD HARM USERS

// Medium

Description

When a new proposal is submitted via the submit_proposal function in the assembly contract, the submitter can introduce a malicious external URL that tricks users into being redirected to a phishing DApp that could steal their funds.

It is worth noting that URLs will appear in the official Astroport frontend, so legitimate users will not be aware if those URLs are malicious or not. In addition, unsanitized title and description fields could lead to other web application exploits such as Cross-site scripting.

Code Location

contracts/assembly/src/contract.rs

    // Validate title
    if title.len() < MIN_TITLE_LENGTH {
        return Err(ContractError::InvalidProposal(
            "Title too short".to_string(),
        ));
    }

    if title.len() > MAX_TITLE_LENGTH {
        return Err(ContractError::InvalidProposal("Title too long".to_string()));
    }

    // Validate the description
    if description.len() < MIN_DESC_LENGTH {
        return Err(ContractError::InvalidProposal(
            "Description too short".to_string(),
        ));
    }
    if description.len() > MAX_DESC_LENGTH {
        return Err(ContractError::InvalidProposal(
            "Description too long".to_string(),
        ));
    }

    // Validate Link
    if let Some(link) = &link {
        if link.len() < MIN_LINK_LENGTH {
            return Err(ContractError::InvalidProposal("Link too short".to_string()));
        }
        if link.len() > MAX_LINK_LENGTH {
            return Err(ContractError::InvalidProposal("Link too long".to_string()));
        }
    }

Score

Impact: 3
Likelihood: 3

Recommendation

PARTIALLY SOLVED: URL and character validation measures were implemented in commit 6910dbf80aba668b4788cc7e73efcf16108cef33. However, Astroport.fi stated that the content of the proposal will be carefully HTML-encoded in a future release of the front-end to mitigate any potential XSS vectors not covered by the smart contract sanitization steps. As the front-end is out of scope for this audit scope and therefore the remediation cannot be reviewed, this issue has been marked as "Partially solved".

8.2 INCONSISTENT XASTRO BLOCK HEIGHT SELECTION

// Medium

Description

The assembly contract uses different block heights when casting a vote and when calculating the total voting power available for a proposal. This situation could allow proposals to bypass the quorum required for approval.

The proposal_quorum is defined as the ratio of total votes to total voting power. However, the calculation of each vote cast and thus total_votes queries XAstro token in proposal.start_block but total_voting_power does so in proposal.start_block - 1, effectively retrieving different states of the XAstro supply. On the other hand, the VxAstro token consistently uses proposal.start_time - 1 in both cases.

To illustrate this issue, take the following example attack scenario which, for simplicity, only considers the XAstro token:

Suppose at block height 100 the XAstro's total supply is 1000, proposal_required_quorum set to 10% and proposal_required_threshold set to 20%.
In the next block (101), more XAstro tokens are minted, taking the total supply to 1500. At this point, a proposal is submitted.
By the end of the voting period, the active proposal will have received 100 votes, 21 of which will be "for" votes.
Finally, end_proposal is called on the aforementioned proposal. proposal_quorum will be 100 / 1000 since proposal.start_block - 1 is requested for the denominator instead of 100 / 1500.

This will unfairly mark the proposal as "Passed", since the quorum check requires a smaller than effective total_voting_power to cast votes.

Code Location

contracts/assembly/src/contract.rs

pub fn calc_voting_power(
    deps: &DepsMut,
    sender: String,
    proposal: &Proposal,
) -> StdResult<Uint128> {
    let config = CONFIG.load(deps.storage)?;

    let xastro_amount: BalanceResponse = deps.querier.query_wasm_smart(
        config.xastro_token_addr,
        &XAstroTokenQueryMsg::BalanceAt {
            address: sender.clone(),
            block: proposal.start_block,
        },

contracts/assembly/src/contract.rs

pub fn calc_total_voting_power_at(deps: &DepsMut, proposal: &Proposal) -> StdResult<Uint128> {
    let config = CONFIG.load(deps.storage)?;

    // Total xASTRO supply at a specified block
    let mut total: Uint128 = deps.querier.query_wasm_smart(
        config.xastro_token_addr,
        &XAstroTokenQueryMsg::TotalSupplyAt {
            block: proposal.start_block - 1,
        },
    )?;

Score

Impact: 3
Likelihood: 3

Recommendation

NOT APPLICABLE: The underlying logic in storing SnapshotMap effectively reflected the user's voting power (calculated at calc_voting_power) in proposal.start_block - 1. Causing the block height to be selected consistently in all the relevant places.

8.3 NO MINIMUM THRESHOLD FOR EFFECTIVE DELAY PERIOD

// Low

Description

Timelocks are defined in the assembly contract to allow users of the protocol to react in time if a change made is bad faith or is not in the best interest of the protocol and its users.

The instantiate and update_config functions of the assembly contract do not restrict timelocks (execution_delay_period and proposal_expiration_period) from being greater than or equal to a minimum threshold. Therefore, malicious changes proposed through voting could even be executed immediately if execution_delay_period is not set appropriately.

Code Location

contracts/assembly/src/contract.rs

let config = Config {
    xastro_token_addr: addr_validate_to_lower(deps.api, &msg.xastro_token_addr)?,
    vxastro_token_addr: addr_validate_to_lower(deps.api, &msg.vxastro_token_addr)?,
    builder_unlock_addr: addr_validate_to_lower(deps.api, &msg.builder_unlock_addr)?,
    proposal_voting_period: msg.proposal_voting_period,
    proposal_effective_delay: msg.proposal_effective_delay,
    proposal_expiration_period: msg.proposal_expiration_period,
    proposal_required_deposit: msg.proposal_required_deposit,
    proposal_required_quorum: Decimal::from_str(&msg.proposal_required_quorum)?,
    proposal_required_threshold: Decimal::from_str(&msg.proposal_required_threshold)?,
};

contracts/assembly/src/contract.rs

if let Some(proposal_effective_delay) = updated_config.proposal_effective_delay {
    config.proposal_effective_delay = proposal_effective_delay;
}

if let Some(proposal_expiration_period) = updated_config.proposal_expiration_period {
    config.proposal_expiration_period = proposal_expiration_period;
}

Score

Impact: 4
Likelihood: 1

Recommendation

SOLVED: The issue was fixed in commit 6dcfefe59514a85495a4e34a2ec50ea41f0d10d2.

8.4 UNCHECKED ARITHMETIC

// Informational

Description

In computer programming, an overflow occurs when an arithmetic operation attempts to create a numeric value that is outside the range that can be represented by a given number of bits – either greater than the maximum value or less than the minimum representable value.

This issue has been raised as informational only, since it was not possible to define a clear exploitation scenario for the affected case.

Code Location

Affected resources

contracts/assembly/src/contract.rs:715:            .checked_add(locked_amount.params.amount - locked_amount.status.astro_withdrawn)?;

Score

Impact: 1
Likelihood: 1

Recommendation

SOLVED: The issue was fixed in commit 6dcfefe59514a85495a4e34a2ec50ea41f0d10d2.

9. Automated Testing

AUTOMATED ANALYSIS

Description

Halborn used automated security scanners to assist with detection of well-known security issues and vulnerabilities. Among the tools used was cargo audit, a security scanner for vulnerabilities reported to the RustSec Advisory Database. All vulnerabilities published in https://crates.io are stored in a repository named The RustSec Advisory Database. cargo audit is a human-readable version of the advisory database which performs a scanning on Cargo.lock. Security Detections are only in scope.

No unmaintained or yanked library was found.

Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.

1. Introduction
2. Audit summary
3. Test approach & methodology
4. Scope
5. Risk methodology
6. Scope
7. Assessment summary & findings overview
8. Findings & Tech Details