g - Valid VFS paths with usernames can always fail validation

[sherlock-admin2]

g

High

Valid VFS paths with usernames can always fail validation

Summary

VFS paths are validated across AndromedaOS with get_raw_address(). Valid VFS paths may include usernames. However, path resolution can fail for paths with valid usernames and in effect, cause validation with get_raw_address() to fail. This issue exists for a large subset of usernames and libraries.

Vulnerability Detail

When get_raw_address() is used to validate a VFS path, it queries the VFS to resolve it. It attempts to resolve the path with resolve_pathname() which eventually calls either resolve_home_path() or resolve_lib_path(). The issue exists in both resolve_lib_path() and resolve_home_path().

When the VFS path includes a registered username or library and that username/library is also a valid address according to deps.api.addr_validate(), the address stored for the registered username/library will not be loaded.

ref: andromeda-vfs/src/state.rs::resolve_home_path()

// @audit-issue if a username is also a valid address, then the address for the registered username can never be loaded
let user_address = match api.addr_validate(username_or_address) {
    Ok(addr) => addr,
    Err(_e) => USERS.load(storage, username_or_address)?, 
};
resolve_path(storage, api, parts, user_address)

The username/library will be used for path resolution instead of the stored address which will cause an error because a non-existent path is being loaded.

ref: andromeda-vfs/src/state.rs::resolve_path()

fn resolve_path(
    storage: &dyn Storage,
    api: &dyn Api,
    parts: Vec<String>,
    user_address: Addr,
) -> Result<Addr, ContractError> {
    let mut address = user_address;
    for (idx, part) in parts.iter().enumerate() {
        if idx <= 1 {
            continue;
        }
        // @audit-issue address here will be the username or library instead of an address. the key is non-existent
        // and will cause an error
        let info = paths().load(storage, &(address, part.clone()))?;

The issue can be verified by changing the username in the test test_resolve_home_path and then running the test with cargo test -- test_resolve_home_path --show-output.

fn test_resolve_home_path() {
    let mut deps = mock_dependencies();
-   let username = "u1";
+   let username = "username1980";

Impact

VFS path validation is done all over AndromedaOS. This issue will break a lot of functionality and cause a loss of funds for the valid paths that are victims of this bug. For example, the Validator Staking ADO does address validation of the recipient in execute_claim(). The recipient that fails validation can never claim their stake. In Kernel ADO, every local AMP message's recipient is validated. This means the victim paths can not receive AMP messages since they will always fail validation. The consequences of this validation issue are far-reaching in the AndromedaOS system and are just a few of the impacts caused.

Code Snippet

• https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/packages/std/src/amp/addresses.rs#L63-L71
• https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/contracts/os/andromeda-vfs/src/state.rs#L57-L77
• https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/contracts/os/andromeda-vfs/src/state.rs#L92-L95
• https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/contracts/os/andromeda-vfs/src/state.rs#L121-L133

Tool used

Manual Review

Recommendation

When resolving the home or lib path, consider checking storage for the username or library. If it exists, then load the address for the username/library. If it does not exist, treat it is an address and validate it with deps.api.addr_validate().

[gjaldon]

Escalate

This is a valid issue because it shows how registered usernames will not be loaded for VFS paths with usernames like /home/username1/app_contract. Usernames that are also valid addresses (they will return true for api.addr_validate()) will not be loaded. Impact is high as explained in the Impact section.

[sherlock-admin3]

Escalate

This is a valid issue because it shows how registered usernames will not be loaded for VFS paths with usernames like /home/username1/app_contract. Usernames that are also valid addresses (they will return true for api.addr_validate()) will not be loaded. Impact is high as explained in the Impact section.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cu5t0mPeo]

this is know issue

[gjaldon]

This is not a known issue. The known issue is that /home/app_contract is used as vfs paths for components of Apps instead of /home/username/app_contract. This issue exists whether path is /home/username/app_contract or /home/app_contract.

[cvetanovv]

@cu5t0mPeo Why do you think it is a known issue?

[cu5t0mPeo]

sry,I was mistaken and thought it was the same issue as the VFS path error.

[cvetanovv]

Watson has shown how valid VFS paths with usernames can always fail validation. This would break a lot of functionalities and lead to loss of funds.

Planning to accept the escalation and make this issue High.

[WangSecurity]

@gjaldon @MxAxM are there any duplicates here?

[WangSecurity]

Result: High Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• gjaldon: accepted

[gjaldon]

@WangSecurity it is unique. Thank you 🙏🏼

[J4X-98]

Hey @cvetanovv @WangSecurity,

Sorry for the late remark on this, I just saw that this was validated. I do not disagree with this issues validity, this is clearly an issue. But I don't see why this was judged as high. There is no loss of funds, this issue is clearly unintended functionality.

The issue states 2 cases as impact:

1. "For example, the Validator Staking ADO does address validation of the recipient in execute_claim(). The recipient that fails validation can never claim their stake."

The impact of not being able to claim can easily be bypassed by the user, by just not passing the optional recipientparameter, in which case the funds will be distributed to the info.sender. This address anyways has to be equal to the recipient due to earlier checks. In this case the reverting call to get_raw_address() will never occur.

let recipient = if let Some(recipient) = recipient {
    recipient.get_raw_address(&deps.as_ref())?
} else {
    info.sender
};

By doing this the user will still be able to retrieve his funds, so no loss of funds. -> Medium

2. "In Kernel ADO, every local AMP message's recipient is validated. This means the victim paths can not receive AMP messages since they will always fail validation."

-> This is just a functionality not working not a loss of funds -> Medium

[gjaldon]

Hi @cvetanovv @WangSecurity @J4X-98,

This issue leads to a loss of funds.

The Vesting ADO's execute_claim() will always fail and the recipient will not be able to claim any of the vested funds. All vested funds will remain stuck in the Vesting ADO because there is no way to change the configured recipient. It is only set on instantiation.

This is an example of loss of funds and there may be other parts in Andromeda where it happens because of this issue since get_raw_address() is used in most, if not all, parts of AndromedaOS (ADO Contracts, Modules, Core contracts).

[J4X-98]

Your issue states that the validator stakings is the one that can't be claimed anymore as you can see in the snippet from your issue below. That's what my first answer was based upon.

For example, the Validator Staking ADO does address validation of the recipient in execute_claim(). The recipient that fails validation can never claim their stake. 

As I have shown that this is not the case, and there is no actual loss of funds, you are now stating that the Vesting's execute_claim() function will not work.

The Vesting ADO's execute_claim() will always fail and the recipient will not be able to claim any of the vested funds. All vested funds will remain stuck in the Vesting ADO because there is no way to change the configured recipient. It is only set on instantiation.

This is also untrue as the vulnerable get_raw_address() function is never called in the vestings execute_claim() function as anyone can easily verify. So the function will never revert due to the issue you describe. Matter of fact, the vulnerable function is never called in the whole vestings contract.rs.

This is an example of loss of funds and there may be other parts in Andromeda where it happens because of this issue since get_raw_address() is used in most, if not all, parts of AndromedaOS (ADO Contracts, Modules, Core contracts).

To answer your second point, stating that there might be some loss of funds but not being able to prove, is not grounds for a high severity.

[gjaldon]

As I have shown that this is not the case, and there is no actual loss of funds, you are now stating that the Vesting's execute_claim() function will not work.

I meant Vesting ADO but made a mistake and wrote Validator Staking. Anyway, I also stated the following in the report:

The consequences of this validation issue are far-reaching in the AndromedaOS system and are just a few of the impacts caused.

This is also untrue as the vulnerable get_raw_address() function is never called in the vestings execute_claim() function as anyone can easily verify. So the function will never revert due to the issue you describe. Matter of fact, the vulnerable function is never called in the whole vestings contract.rs.

get_raw_address() is actually called indirectly via generate_direct_msg(). The Vesting ADO's execute_claim() calls generate_direct_msg() here.

[gjaldon]

Almost everywhere an AndrAddr is accepted get_raw_address() will be called to validate it.

[J4X-98]

You're right about the function being called via the other one, I missed that.

Nevertheless I think it's important to add that this issue in vesting is not a permanent loss of funds as the vesting contract has a migrate function which allows the owner to migrate to a version including a function that allows him to withdraw the tokens again. As the owner is also the one the funds originate from, this issue just results in him not being able to distribute the rewards to users affected by this issue. -> this is unintended functionality not loss of funds.

This is in contrast to the staking module, that does not allow migrating so there the funds can not be rescued in this way.

[gjaldon]

It will lead to loss of funds because will be sent to the incorrect address.

When resolving the path "/home/username1" with get_raw_address(), it will return "username1" as the address. execute_claim() will send funds to "username1" which is a valid address and the funds will be lost.

Function flow for get_raw_address() on "/home/username1":

1. local_path_to_vfs_path() will return "/home/username1" since it isn't a local path.
2. vfs_resolve_path() will then be called which will call resolve_pathname() --> resolve_home_path() --> resolve_path()
3. "username1" will be passed as the user address to resolve_path() because it is a valid address as stated in the original report.
4. resolve_path() returns "username1" as the address because the "home" and "username1" parts of the path will be skipped.

=====================

This is in contrast to the staking module, that does not allow migrating so there the funds can not be rescued in this way.

Regarding Staking ADO's upgradability, not having a migrate function defined in Staking ADO does not mean it is not upgradeable. The admin only needs to be set for the contract and the new contract is the one that needs to implement the migrate function. Basically, all contracts in CosmWasm chains are upgradeable if an admin is set which is up to deploy ops.

From CosmWasmMigration docs:

During the migration process, the migrate function defined in the new contract is executed and not the migrate function from the old code and therefore it is necessary for the new contract code to have a migrate function defined and properly exported as an entry_point:

[J4X-98]

First you stated it will revert, now you state that it will be transferred to the wrong address. Which one is the case now?

[gjaldon]

This issue is high severity because of the loss of funds.

"./username1" paths will lead to reverts and "/home/username1" paths will lead to transfers to the incorrect address.

The original report states:

The consequences of this validation issue are far-reaching in the AndromedaOS system and are just a few of the impacts caused.

There are multiple impacts caused by the issue and are not limited to the ones I provided as stated in the original report. The report did state the impact is loss of funds.

[J4X-98]

But wouldn't this require a user to set a malicious name and also require the owner to set a malicious name as the recipient. I guess we can assume that if someone uses "/home/username1" as his username this is clearly a payload to everyone setting this somewhere.

[gjaldon]

No malicious username needs to be used. The vulnerability is naturally occurring for valid usernames.

[J4X-98]

Would you define "/home/username" as a non-malicious username?

[gjaldon]

Yes. That's only an example and there are many other usernames. Anyway, I've provided enough explanation to show that the issue exists and leads to permanent loss of funds and other impacts. I've spent too much time on this discussion already. I hope you don't mind that we end the discussion here.

[cvetanovv]

Judging by this comment here, I'm planning to change the severity of #41 and #30 to Medium because the funds can be rescued.

@gjaldon is right that in CosmWasm, contracts can be set upgradeable by the Admin at the beginning. According to the rules, the Admin is expected to take the correct action. It might also mean setting the contracts upgradeable. This is missing as QA in the Readme, so we'll stick to assuming the Admin will make the right decisions.

[gjaldon]

Hi @cvetanovv @WangSecurity. The loss of funds in this report is permanent because they will be sent to the incorrect address.

I explained it in this comment.

Please let me know if there is anything that is unclear.

[J4X-98]

The case you describe would require the user to provide a malicious username formed like a path to lose his own rewards. This scenario makes no sense to me.

I agree with @cvetanovv judging

[gjaldon]

The case you describe would require the user to provide a malicious username formed like a path to lose his own rewards. This scenario makes no sense to me.

This is a high-severity issue that causes permanent loss of funds for valid usernames. There are no "malicious" usernames that need to be used. Users only need to register and use valid usernames and they end up losing funds.

I'm unsure how else to explain the issue to make it clearer for you @J4X-98.

[J4X-98]

I'm aware of the issue you describe. However, it requires someone to register with a username like "/home/username/" so that the resolving fails, which makes no sense unless the user wants to bypass this feature. This makes no sense for a user, because it will just result in him losing his own rewards.

[gjaldon]

@J4X-98 can you read the code about VFS Path resolution first before arguing here? It's a waste of time to have to explain.

When registering usernames, a user only needs to register a username like "abcde1". Now when paths are resolved, they are expected to be either just "home" or "lib" paths. Home paths are prefixed by "~/" or "/home" while lib paths are prefixed by "/lib".

Code in path resolution:

    match pathname.get_root_dir() {
        "home" => resolve_home_path(storage, api, pathname),
        "lib" => resolve_lib_path(storage, api, pathname),
        &_ => Err(ContractError::InvalidAddress {}),
    }

The only way to use the username "abcde1" in a VFS path is to prefix it with "/home".

Loading username:

fn resolve_home_path(
    storage: &dyn Storage,
    api: &dyn Api,
    pathname: AndrAddr,
) -> Result<Addr, ContractError> {
    // snip ...
    let user_address = match api.addr_validate(username_or_address) {
        Ok(addr) => addr,
        Err(_e) => USERS.load(storage, username_or_address)?, // The address linked to the username is loaded here
    };

So your argument below makes ZERO sense:

However, it requires someone to register with a username like "/home/username/" so that the resolving fails, which makes no sense unless the user wants to bypass this feature.

No need to register "/home/username". They only need to register "username" and using usernames in VFS paths means the path always needs to be prefixed "/home". That's how VFS paths work in AndromedaOS.

I want to be as civil and respectful as possible and play fair, but you seem to be intentionally wasting people's time here @J4X-98.

[J4X-98]

Hey @gjaldon ,

I will refrain from answering your insults regarding wasting everyone's time and will try to stay factual here.

Based on your issue and the following comments, you state that if a user uses the username "username1" and a vesting is generated for him, he will not receive the tokens due to the VFS path being incorrectly resolved. In your example the vesting should be generated with the VFS path as the recipient being set.

I have adapted the testcases in the vesting module to this scenario, and the user exactly receives the tokens to the provided vfs path as you can see from the returned Send message. Could you show me exactly where the vulnerability occurs here / tokens are lost? To me it seems like the vesting system does exactly what it should.

fn init(deps: DepsMut) -> Response {
    let msg = InstantiateMsg {
        recipient: Recipient::from_string("/home/username1"), //Vesting is generated with the VFS path you describe as the recipient
        is_multi_batch_enabled: true,
        denom: "uusd".to_string(),
        unbonding_duration: Duration::Height(UNBONDING_BLOCK_DURATION),
        kernel_address: MOCK_KERNEL_CONTRACT.to_string(),
        owner: None,
        modules: None,
    };

    let info = mock_info("owner", &[]);
    instantiate(deps, mock_env(), info, msg).unwrap()
}

#[test]
fn test_claim_batch_single_claim() {
    let mut deps = mock_dependencies_custom(&[]);
    init(deps.as_mut());
    let info = mock_info("owner", &coins(100, "uusd"));

    let release_unit = 10;

    // Create batch.
    let msg = ExecuteMsg::CreateBatch {
        lockup_duration: None,
        release_unit,
        release_amount: WithdrawalType::Amount(Uint128::new(10)),
        validator_to_delegate_to: None,
    };

    let _res = execute(deps.as_mut(), mock_env(), info.clone(), msg).unwrap();

    deps.querier
        .base
        .update_balance(MOCK_CONTRACT_ADDR, coins(100, "uusd"));

    // Skip time.
    let mut env = mock_env();
    // A single release is available.
    env.block.time = env.block.time.plus_seconds(release_unit);

    // Query created batch.
    let msg = QueryMsg::Batch { id: 1 };
    let res: BatchResponse = from_json(query(deps.as_ref(), env.clone(), msg).unwrap()).unwrap();

    let lockup_end = mock_env().block.time.seconds();
    assert_eq!(
        BatchResponse {
            id: 1,
            amount: Uint128::new(100),
            amount_claimed: Uint128::zero(),
            amount_available_to_claim: Uint128::new(10),
            number_of_available_claims: Uint128::new(1),
            lockup_end,
            release_unit,
            release_amount: WithdrawalType::Amount(Uint128::new(10)),
            last_claimed_release_time: lockup_end,
        },
        res
    );

    // Claim batch.
    let msg = ExecuteMsg::Claim {
        number_of_claims: None,
        batch_id: 1,
    };

    let res = execute(deps.as_mut(), env, info, msg).unwrap();

    assert_eq!(
        Response::new()
            .add_message(BankMsg::Send {
                to_address: "/home/username1".to_string(), // Tokens are sent to the correct address
                amount: coins(10, "uusd")
            })
            .add_attribute("action", "claim")
            .add_attribute("amount", "10")
            .add_attribute("batch_id", "1")
            .add_attribute("amount_left", "90"),
        res
    );
    let lockup_end = mock_env().block.time.seconds();

    assert_eq!(
        Batch {
            amount: Uint128::new(100),
            amount_claimed: Uint128::new(10),
            lockup_end,
            release_unit: 10,
            release_amount: WithdrawalType::Amount(Uint128::new(10)),
            last_claimed_release_time: lockup_end + release_unit,
        },
        batches().load(deps.as_ref().storage, 1u64).unwrap()
    );
}

You can add this test to contracts/finance/andromeda-vesting/src/testing/tests.rs to verify yourself that it passes.

[gjaldon]

@J4X-98,

Your test case does not prove the issue does not exist. It is also incomplete since the username "username1" is not registered.

I have adapted the testcases in the vesting module to this scenario, and the user exactly receives the tokens to the provided vfs path as you can see from the returned Send message

In your test, the tokens are sent to the address "/home/username1". When a VFS Path is resolved, it is supposed to load the address linked to a registered username and not to the actual path "/home/username1". That is exactly what this report points out as the issue.

For example:

1. Alice registers the username "imalice1" linked to the address "valid_address".
2. Alice provides the VFS Path "/home/imalice1" as recipient when claiming.
3. "valid_address" is supposed to receive the tokens but they are sent to the address "imalice1".

Does that clarify the issue?

[cvetanovv]

@gjaldon I spoke with the sponsor, and they confirmed that it is indeed a valid attack vector, however, it is a known issue from a previous audit. You can check out this audit - AND-42.

Because of this, I plan to invalidate the issue.

[gjaldon]

@cvetanovv AND-42 is different from this report. AND-42 states:

Of note, the function resolve_home_path() in contracts/os/andromeda-vfs/src/state.rs appears to handle this possibility correctly to prevent spoofing, by first matching on a valid address before doing a username lookup:

let user_address = match api.addr_validate(username_or_address) { Ok(addr) => addr, Err(_e) => USERS.load(storage, username_or_address)?, };

AND-42 states that those lines of code are correctly handling the prevention of spoofing. This report, on the other hand, points out that there is an issue in those same lines of code which leads to a vulnerability.

// @audit-issue if a username is also a valid address, then the address for the registered username can never be loaded
let user_address = match api.addr_validate(username_or_address) {
    Ok(addr) => addr,
    Err(_e) => USERS.load(storage, username_or_address)?, 
};
resolve_path(storage, api, parts, user_address)

AND-42 points out that no validation prevents registering a username with an address that is not their own. However, that is no longer possible in the code in the audit scope.

This report is different since it is not about being able to register a username with an address that is not the registrant's. That is no longer possible for a user to do in the audited code. This report is about VFS Path resolution not returning the registered address for the username.

I explain the behavior here and the test case here actually confirms it.

[WangSecurity]

Based on this comment, isn't it Alice's mistake that she entered an incorrect data, firstly? And secondly, in that scenario the funds are still sent to her address, so the funds are not lost and she received them? I think entering one of your two addresses but receiving your funds on the another address of yours is not a loss?

And about the attack path in that comment. Firstly, you say "imalice1" is a username, but then you say it's an address, so why then not input the intended address "valid_address" initially? Moreover, it's not in the report and the two impacts explained in the report are not high severity impact, since the first leads to users not being able to claim the rewards, but the contract is upgradeable so the admin can retrieve the funds and the second about not receiving messages I believe is also medium.

[WangSecurity]

During the discussion in Discord with @gjaldon we reached an agreement that this indeed has to be Med, and will downgrade the severity to M in a couple of hours