g - Permission checks will unnecessarily consume Limited uses

[sherlock-admin2]

g

Medium

Permission checks will unnecessarily consume Limited uses

Summary

When a permission check is done with ado_contract/permissioning.rs::is_permissioned() on a Limited permission, uses are always consumed.

Vulnerability Detail

When doing a permission check, a Limited permission will always have its uses consumed.

ref: ado_contract/permissioning.rs::is_permissioned()

pub fn is_permissioned(
    // ... snip ...
) -> Result<(), ContractError> {
    // ... snip ...
    match permission {
        Some(mut permission) => {
            // ... snip ...
            if let Permission::Limited { .. } = permission {
                permission.consume_use();
                permissions().save(
                    store,
                    (action_string.clone() + actor_string.as_str()).as_str(),
                    &PermissionInfo {
                        action: action_string,
                        actor: actor_string,
                        permission,
                    },
                )?;
            }

            Ok(())
        }

In functions like is_context_permissioned() and is_context_permissioned_strict(), the permission check may be done on 2 different users. Consider the case when both the origin and the previous sender have Limited permissions. Both of their Limited uses will be consumed even when permissions for one is enough.

ref: ado_contract/permissioning.rs::is_context_permissioned()

pub fn is_context_permissioned(
    // ... snip ...
) -> Result<bool, ContractError> {
    let contract = ADOContract::default();

    match ctx {
        // auth is done on origin and previous_sender
        Some(amp_ctx) => {
            let action: String = action.into();
            let is_origin_permissioned = contract.is_permissioned(
                storage,
                env.clone(),
                action.clone(),
                amp_ctx.ctx.get_origin().as_str(),
            );
            let is_previous_sender_permissioned = contract.is_permissioned(
                storage,
                env.clone(),
                action,
                amp_ctx.ctx.get_previous_sender().as_str(),
            );
            Ok(is_origin_permissioned.is_ok() || is_previous_sender_permissioned.is_ok())

In the cw721 ADO Contract, is_context_permissioned() is called for every execute handling and every mint and batch mint will call is_context_permissioned_strict(). This leads to minting and batch minting consuming up to 4 Limited uses across 2 addresses.

Impact

Users/Contracts with Limited permissions will unexpectedly run out of uses. A whitelisted user who notices this behavior can use up a contract's or user's limited uses by using AMP to have the target address be the origin or previous sender. This issue can also naturally occur.

Code Snippet

• https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/packages/std/src/ado_contract/permissioning.rs#L43-L91
• https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/packages/std/src/ado_contract/permissioning.rs#L323-L353
• https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/packages/std/src/ado_contract/permissioning.rs#L360-L390

Tool used

Manual Review

Recommendation

Consider ensuring that the is_permissioned() check only consumes the Limited use of just one address. Also, it may be worth considering changing the minting functions in cw721 ADO contract to only call is_context_permissioned_strict() and not call is_context_permissioned().

[gjaldon]

Escalate

This report shows that users with Limited permissions will run out of uses sooner than expected, since the expectation is the one permissioned action will only consume 1 use and not 2-4. The issue naturally occurs and a Whitelisted user can force consumption of users with Limited uses.

[sherlock-admin3]

Escalate

This report shows that users with Limited permissions will run out of uses sooner than expected, since the expectation is the one permissioned action will only consume 1 use and not 2-4. The issue naturally occurs and a Whitelisted user can force consumption of users with Limited uses.

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.

[J4X-98]

This issue consist of 2 root causes:

is_context_permissioned() consumes a user from the origin and sender

• This is clearly intended design. If this would have not been intended, the developers would not have added the whole additional check. There is also no documentation stating that it should not consume the origins user too.

cw721 burns too many uses

• This is a vulnerability in the cw721 contract where the usage of permissions was incorrectly implemented. cw721 is not in scope. So a wrong usage of the correctly working is_permissioned functions (that are in scope) in a different out of scope file can not be considered an issue.

[cvetanovv]

@gjaldon @J4X-98 @MxAxM Is cw721 a library?

That makes cw721 in scope according to the rules and is the opposite of what I wrote under another issue.

[J4X-98]

Hey @cvetanovv ,

cw721 is not a library used by any of the modules or the core contract which are in scope. It is an additional module that inherits some of the core functionalities. So the inheritance is not in the way which would put it in scope as per the rules, but the different way around.

[gjaldon]

@cvetanovv @J4X-98 the issue is also in https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/packages/std/src/ado_contract/permissioning.rs#L43-L91 which is a file in-scope. The issue happens in the following lines:

      let is_origin_permissioned = contract.is_permissioned(
          storage,
          env.clone(),
          action.clone(),
          amp_ctx.ctx.get_origin().as_str(),
      );
      let is_previous_sender_permissioned = contract.is_permissioned(
          storage,
          env.clone(),
          action,
          amp_ctx.ctx.get_previous_sender().as_str(),
      );

is_permissioned() is called on both the previous sender and the origin. Given those, the limited uses are already consumed for 2 different users.

The issue in cw721 adds 2 more uses. So even without the issue in cw721, is_permissioned() still needs fixing. The permissioning functions are only used in the cw721 contract and not in the contracts in-scope (vesting and validator-staking).

[gjaldon]

Also, the following argument has no bearing.

This is clearly intended design. If this would have not been intended, the developers would not have added the whole additional check. There is also no documentation stating that it should not consume the origins user too.

It can also be argued that no documentation states that limited uses should be consumed for both the origin and the previous_sender.

Also, consider the scenario where origin and previous_sender are the same address (a common case). That address will get its 2 limited uses consumed to access one action. This example more clearly shows that this behavior is incorrect.

[cvetanovv]

I have received confirmation from the sponsor that this is not an intended design and is indeed an issue.

is_permissioned() is within the audit scope, so I plan to accept the escalation.

[WangSecurity]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• gjaldon: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/andromedaprotocol/andromeda-core/pull/553

[bin2chen66]

fix-reviews note: https://github.com/andromedaprotocol/andromeda-core/pull/553 https://github.com/andromedaprotocol/andromeda-core/pull/569 PR modified is_context_permissioned and is_context_permissioned_strict() to execute only once to reduce the number of times Fixed this issue