Permissioned actions can not be disabled

Summary

ADO contracts can set permissioned actions. However, once they are set, they can not be disabled.

Vulnerability Detail

Every ADO contract has an execute handler that handles the AndromedaMsg::PermissionAction message. This sets the given action as permissioned.

ref: std/src/ado_contract/execute.rs::execute()

AndromedaMsg::PermissionAction { action } => {
    self.execute_permission_action(ctx, action)
}

It updates the permissioned_actions with permission_action().

ref: std/src/ado_contract/permissioning.rs::permission_action()

    pub fn permission_action(
        &self,
        action: impl Into<String>,
        store: &mut dyn Storage,
    ) -> Result<(), ContractError> {
        self.permissioned_actions
            .save(store, action.into(), &true)?;
        Ok(())
    }

The PermissionAction only ever sets an action as permissioned. To disable it, execute_disable_action_permission() must be called—however, no ADO contract handler calls it.

Impact

Once actions are permissioned in an ADO contract like the CW721 ADO, there is no way for the owner to open up the actions to all users. For example, the owner makes NFT Transfers in the CW721 ADO as a permissioned action. Once done, there is no way for NFT sellers and buyers to transact without the owner manually whitelisting them. The contract becomes unusable since buyers and sellers can not freely transact.

Code Snippet

Tool used

Manual Review

Recommendation

Consider

gjaldon commented 3 months ago

Escalate

ADO contract owners can make an action permissioned but will not be able to revert it to non-permissioned. There is a function for disabling the permissioned action (execute_disable_action_permission()) but there is no handler for it.

Since the function exists, it's clear the intent was to implement a handler for it.

sherlock-admin3 commented 3 months ago

Escalate

ADO contract owners can make an action permissioned but will not be able to revert it to non-permissioned. There is a function for disabling the permissioned action (execute_disable_action_permission()) but there is no handler for it.

Since the function exists, it's clear the intent was to implement a handler for it.

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 commented 3 months ago

The issue does exist.but owner is a trusted role, so permitted action should be secure.

cvetanovv commented 3 months ago

I disagree with the escalation. In the Readme, the protocol wrote that the Owner is TRUSTED. Therefore, the issue is invalid.

Planning to reject the escalation and leave the issue as is.

WangSecurity commented 3 months ago

Result: Invalid Unique

sherlock-admin4 commented 2 months ago

Escalations have been resolved successfully!

Escalation status:

gjaldon: rejected

sherlock-audit / 2024-05-andromeda-ado-judging

g - Permissioned actions can not be disabled #60