bin2chen - is_permissioned() It doesn't make sense to have permissions by default after Blacklisted expires.

[sherlock-admin4]

bin2chen

Medium

is_permissioned() It doesn't make sense to have permissions by default after Blacklisted expires.

Summary

in is_permissioned(), returns true if Permission::Blacklisted has expired
it is not correct

Vulnerability Detail

in is_permissioned() to determine if a permission is granted.

    pub fn is_permissioned(&self, env: &Env, strict: bool) -> bool {
        match self {
            Self::Blacklisted(expiration) => {
                if let Some(expiration) = expiration {
                    if expiration.is_expired(&env.block) {
@>                       return true;
                    }
                }
                false
            }
            Self::Limited { expiration, uses } => {
                if let Some(expiration) = expiration {
                    if expiration.is_expired(&env.block) {
@>                      return !strict;
                    }
                }
                if *uses == 0 {
                    return !strict;
                }
                true
            }
            Self::Whitelisted(expiration) => {
                if let Some(expiration) = expiration {
                    if expiration.is_expired(&env.block) {
                        return !strict;
                    }
                }
                true
            }
        }
    }

The current implementation returns true if the blacklist has expired, regardless of strict.
The following scenarios are problematic

1. action1 doesn't need permission at the beginning, i.e.: strict = false
2. the administrator has blacklisted alice for 1 month, i.e.: alice has Permission::Blacklisted
3. after some time (> 1 month)
4. the administrator changes the permissions configuration of action1 to action1 requires permissions, i.e.: strict = true
5. at this point is_permissioned(alice) returns true, and alice becomes permitted by default, which is not correct!

It is reasonable to return !strict when it expires, just like Limited and Whitelisted.

Impact

Permission::Blacklisted expires and returns true, causing users to have permissions that shouldn't have them.

Code Snippet

https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/packages/std/src/ado_base/permissioning.rs#L55

Tool used

Manual Review

Recommendation

    pub fn is_permissioned(&self, env: &Env, strict: bool) -> bool {
        match self {
            Self::Blacklisted(expiration) => {
                if let Some(expiration) = expiration {
                    if expiration.is_expired(&env.block) {
-                       return true;
+                       return  !strict;
                    }
                }
                false
            }
            Self::Limited { expiration, uses } => {
                if let Some(expiration) = expiration {
                    if expiration.is_expired(&env.block) {
                        return !strict;
                    }
                }
                if *uses == 0 {
                    return !strict;
                }
                true
            }

[J4X-98]

Escalate

This is intended behavior. The documentation (code comments) describes the intended behavior as follows:

/// An enum to represent a user's permission for an action
///
/// - **Blacklisted** - The user cannot perform the action until after the provided expiration
/// - **Limited** - The user can perform the action while uses are remaining and before the provided expiration **for a permissioned action**
/// - **Whitelisted** - The user can perform the action until the provided expiration **for a permissioned action**
///
/// Expiration defaults to `Never` if not provided
#[cw_serde]
pub enum Permission {
    Blacklisted(Option<Expiration>),
    Limited {
        expiration: Option<Expiration>,
        uses: u32,
    },
    Whitelisted(Option<Expiration>),
}

The documentation clearly states that the intended behavior for a blacklisted user is "The user cannot perform the action until after the provided expiration". So the user should be able to regain possibility to take actions after expiry which is the whole "vulnerability" described by this group of issues.

[sherlock-admin3]

Escalate

This is intended behavior. The documentation (code comments) describes the intended behavior as follows:

/// An enum to represent a user's permission for an action
///
/// - **Blacklisted** - The user cannot perform the action until after the provided expiration
/// - **Limited** - The user can perform the action while uses are remaining and before the provided expiration **for a permissioned action**
/// - **Whitelisted** - The user can perform the action until the provided expiration **for a permissioned action**
///
/// Expiration defaults to `Never` if not provided
#[cw_serde]
pub enum Permission {
    Blacklisted(Option<Expiration>),
    Limited {
        expiration: Option<Expiration>,
        uses: u32,
    },
    Whitelisted(Option<Expiration>),
}

The documentation clearly states that the intended behavior for a blacklisted user is "The user cannot perform the action until after the provided expiration". So the user should be able to regain possibility to take actions after expiry which is the whole "vulnerability" described by this group of issues.

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.

[cvetanovv]

I don't see a problem here. NatSpec comments highlight that when a user is blacklisted, it is expected after a while to be able to regain the possibility to take action after the restriction expires.

Planning to accept the escalation and invalidate the issue.

[bin2chen66]

/// - Blacklisted - The user cannot perform the action until after the provided expiration

The way I understand this sentence is.

Suppose action is permissionless (strict=false) and can be executed by anyone. But the administrator can restrict someone from performing it, for example by blacklisting alice so that alice cannot perform the action until after the provided expiration.

Example: Suppose withdrawal doesn't need permissions, but the administrator finds out that alice is acting maliciously and blacklists alice for 1 month, so that alice can't withdraw funds Within this month.

But the problem is, if later the administrator decides to make the action withdrawal require permissions by default, i.e.: change strict=true.

But, since alice was previously blackballed and expiration, alice now becomes permissioned instead (Current code returns true.). This does not make sense.

So the blacklisting mechanism, like the other two Limited and Whitelisted, should expire as if it was not set, and go back to the way it was, i.e. !strict, instead of returning true.

[J4X-98]

Hey @bin2chen66 ,

There is no default limitation state. As you can see on both the other roles, they also return true if they are not limited by expiration/uses. So effectively blacklist goes to the "default" state after expiration.

[bin2chen66]

hey @J4X-98

sorry , I didn't get what you meant.

if strict=true

current code:
Limited expiration return !strict = false (No permission)
Whitelisted expiration return !strict = false (No permission)
Blacklisted expiration return true (have permission) --> This is the key point, if it expires it should be the same as if it wasn't set up, and doesn't have permissions

[J4X-98]

Hey @bin2chen66 ,

All the functions only return '!strict' if the user is not allowed by either their whitelist having expired or them not having any uses left. Otherwise the 2 others also return true by default. So there seem to only be 2 intended states:

1. user is not allowed -> return '!strict'
2. User is allowed -> return 'true'

And the implementation implements this as intended.

The mediation you recommend would lead to the user still not having permissions after the blacklist expired, leading to a conflict with the description of the functionality.

[bin2chen66]

hey @J4X-98
Returning true is only good for the strict=false case
In the case of strict=true, returning true would be a serious security issue
That's what this issues mentions.and the sponsor seems to have confirmed this question: Labels :“Sponsor Confirmed "

[J4X-98]

Hey @bin2chen66 ,

What you recommend is returning !strict, which is the same behavior as when the blacklisted has not expired. This collides with the intended behavior of the user, regaining the possibility of action. The current behavior is

1. Blacklist is not expired:

• strict -> return false
• !strict-> return true

2. Blacklist is expired:

• strict-> return true
• !strict-> return true

So as a result, the user regains access to strict functions when the blacklist has expired, which adheres to the behavior described by the documentation.

Your proposed mitigation would result in the function always returning !strict, doesn't matter if the blacklist has expired or not, which would make the whole expiration process redundant and would not adhere to the documentation. The function is currently implemented in the way it is documented and does not lead to any vulnerabilities. You are describing intended behavior as a vulnerability, and your mitigation would break intended behavior.

[gjaldon]

The statement below from this comment is incorrect:

So effectively blacklist goes to the "default" state after expiration.

The "default" state is no access and not upgraded access.

Also, the issue is that when the blacklist expires, the user's permission is upgraded to the level of a non-expired whitelisted user. A non-expired whitelisted user has access even to strict permissioned actions. It does not make sense for a blacklisted user to become whitelisted on expiration. In the NFT ADO Contract, this would mean the expired blacklisted user can arbitrarily mint NFTs like the minter role.

[J4X-98]

Hey, what you are missing is that a user can only be in two states as the current implementation works. These are either returning !strict or true all the time.

"The "default" state is no access and not upgraded access." -> The default state (which does not exist) you describe always returns false. So in your opinion, while a user is blacklisted, he should be able to access some functionalities that are not strict. Once the blacklist expires, the user should not be able to access anything anymore. This makes no sense and clashes with the documentation of the user regaining access once the blacklist has expired.

It might be a future improvement to change the whole system to add more states that allow for more detailed control of the user's permissions. But for the current states, the contract can choose from there are only 2, and the implementation correctly uses one of those (the restricted one) while the user is blacklisted and the other one (no restrictions) once the blacklist has expired. This confirms the documentation, so there is no vulnerability; it is just a recommendation for future improvement.

[cvetanovv]

I agree with @J4X-98 comments that this is expected behavior. My early decision remains to accept the escalation and invalidate the issue.

[gjaldon]

@cvetanovv although the comment is correct, it does not completely describe the current behavior of blacklisting.

Blacklisted - The user cannot perform the action until after the provided expiration

After the blacklisting expires, the user gains whitelisted privileges. It's unclear just based on the comment if the dev team intends to increase a user's permissions to greater than a regular user once their blacklist expires.

Also, isn't the dev team fixing the issue? Doesn't that confirm it?

[J4X-98]

As described in multiple of the messages above there is no default behavior. There only is returning !strict or true and those are correctly used by the contract.

Repeating the same invalid argument countless times does not make it more true. The judge has already twice confirmed that this issue is invalid, I don't see a reason to continue this discussion.

[gjaldon]

Hi @J4X-98. Thank you for your opinions.

I presented 2 new arguments which are:

• The comment does not completely describe the current behavior of blacklisting
• The dev team is fixing the issues, which seems to confirm it

The only way to know the intended functionality is to ask the dev team, especially since it is tagged as Will Fix.

[J4X-98]

Hey, I think the comment pretty clearly describes the intended behavior "Blacklisted - The user cannot perform the action until after the provided expiration". The user should not be able to access permissioned functions while blacklisted and should be able to do so once the blacklist has expired. This is exactly what happens in the code.

[gjaldon]

But does it make sense for a previously blacklisted user's privileges to be escalated to the level of a whitelisted user? The comment does not mention anything about that.

It looks like an unintended privilege escalation.

[cu5t0mPeo]

This seems to be a mistake. I asked the sponsor, but it was after the competition.

[gjaldon]

I really appreciate your input, @cu5t0mPeo. 🙏🏼

The sponsor has confirmed this as an issue @cvetanovv. It is an unintended privilege escalation. Sorry for all the discussion. Hope that's enough to validate the issue and reject the escalation.

[cvetanovv]

After the sponsor confirms that this is not an intended design, I will reject the escalation and leave the issue as is.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• J4X-98: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
andromedaprotocol/andromeda-core#553

[bin2chen66]

fix-reviews note:
andromedaprotocol/andromeda-core#553
This PR modifies the blacklist expiration to return !strict as expected
Fixed this issue