Fix: AuthControlled faucet leaves Authority setters unauthenticated#2958
Conversation
…' into fix-h1-unauthenticated-authority
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one comment inline about tightening up valid combinations of AccessControl and AuthMethod enums.
| let trigger_procedures = if is_auth_controlled { | ||
| all_authority_gated_setter_roots() | ||
| } else { | ||
| vec![FungibleFaucet::mint_and_send_root()] | ||
| }; |
There was a problem hiding this comment.
Is is_auth_controlled == false a valid value here? I think of the valid combinations as follows:
- If
AccessControl == AuthControlled, we are dealing with a user account and then validAuthMethods would beSingleSigandNoAuth(mostly for testing purposes). - If
AccessControlis anything else, we are dealing with a network account, and then the validAuthMethodwould beNetworkAccount(maybeNoAuthtoo - but I'm actually not sure about this).
There was a problem hiding this comment.
I think this problem will be better solved once we've done the refactor here: #2944
I have temporarily added a build_auth_component method, which I think it is a weak solution, we should consider removing it in the merging AuthMethod with AccessControl PR.
And, the other solution will be to this once we have separate create_network_fungible_faucet and create_user_fungible_faucet specified here: #2944 (comment)
There was a problem hiding this comment.
Yes, agreed. What we have has issues but works for now. We should definitely refactor this though to make the structure more intuitive, and ideally, make invalid combinations impossible by construction. #2944 would be a good place to do it.
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
| /// Every authority-gated procedure root that must require a signature when | ||
| /// [`AccessControl::AuthControlled`] is paired with [`AuthMethod::SingleSig`]. Includes | ||
| /// `mint_and_send` so that minting always requires a signature regardless of the access | ||
| /// control variant. | ||
| fn all_authority_gated_setter_roots() -> Vec<AccountProcedureRoot> { | ||
| vec![ | ||
| FungibleFaucet::mint_and_send_root(), | ||
| FungibleFaucet::set_max_supply_root(), | ||
| FungibleFaucet::set_description_root(), | ||
| FungibleFaucet::set_logo_uri_root(), | ||
| FungibleFaucet::set_external_link_root(), | ||
| TokenPolicyManager::set_mint_policy_root(), | ||
| TokenPolicyManager::set_burn_policy_root(), | ||
| TokenPolicyManager::set_send_policy_root(), | ||
| TokenPolicyManager::set_receive_policy_root(), | ||
| ] | ||
| } |
There was a problem hiding this comment.
Not for this PR, but I think a safer design for AuthSinglesigAcl might be something similar as what we recently did for AuthMultisig, where if a procedure is called that is not configured, the threshold is set to at least default_threshold.
Similarly for the ACL, we could flip the current logic and say that only configured procedures are exempt from a signature. This is the safer default. E.g. you could say receive_asset can run without signature, but anything else requires a signature.
It would also make the configuration less cumbersome, e.g. here we need to carefully list all procedures exhaustively and if we forget just one, we might have a security hole.
I'm not yet sure how to marry this concept together with with_allow_unauthorized_input_notes and the with_allow_unauthorized_output_notes, but maybe we'd have to remove this and require users to be more explicit (e.g. explicitly configure receive_and_burn as a no-signature procedure).
There was a problem hiding this comment.
I think we can resolve this as described in the second approach here: #2964 (comment)
…' into fix-h1-unauthenticated-authority
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one comment inline - once it is addressed, we can merge (but would be good for @PhilippGackstatter to take another look).
| // AuthControlled + NetworkAccount: accepted; allowed_script_roots is the caller's | ||
| // responsibility (must not include scripts that can invoke authority-gated setters). | ||
| (AccessControl::AuthControlled, AuthMethod::NetworkAccount { allowed_script_roots }) => { |
There was a problem hiding this comment.
I don't think this is a valid combination: a network account needs either Ownable2Step or Rbac access control approaches.
There was a problem hiding this comment.
The comment explicitly states that figuring out which notes are safe "is the caller's responsibility".
I'm not fully convinced this is invalid
There was a problem hiding this comment.
I've fine with keeping it for now - but I do think this is a weird combination. Maybe naming is off here - but AuthControlled really means "user-controlled" to me, and "user-controlled network account" doesn't make too much sense.
There was a problem hiding this comment.
So I think it is generally a valid combination, but that said: disallowing it is a good safety net.
Usually, we expect the operator of the network account to explicitly only allow consumption of scripts they have audited and vetted as safe.
But, if for some reason, they fail to assert the script's safety:
- they missed the suspicious call during the audit
- they accidentally / under pressure updated the allowlist to include the malicious script
then there are no guards against the script invoking e.g. set_mint_policy(AllowAll), whereas with Ownable2Step we still have the owner checks on the critical procedures
4 participants
This PR closes #2943.
The root cause was that
create_fungible_faucetconfiguredAuthSingleSigAclwith onlymint_and_sendin its trigger list, leaving every other authority-gated setter (set_max_supply, the token metadata setters, and the policy-managerset_*_policyprocedures) permissionless underAccessControl::AuthControlled. The fix has two parts:Trigger list expansion: under
AccessControl::AuthControlled+AuthMethod::SingleSig, theAuthSingleSigAcltrigger list now contains every authority-gated setter exported by the faucet, so each one requires a signature. Other access-control variants keep the original[mint_and_send]trigger list because their setter gate is enforced in-procedure.Invalid combinations are rejected:
(AuthControlled, NoAuth)would have left setters permissionless even with the expanded trigger list.all_authority_gated_setter_roots()helper as the single source of truth for the trigger list underAuthControlled, keeps the security invariant in one place and prevents possible mistakes if more setters are added.