diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc022b9f..6e331377 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,9 @@ jobs: - name: Run tests run: cargo test --all + - name: Check for circular crate dependencies (#770) + run: python3 scripts/check_crate_cycles.py + - name: Run multi-sig tests if: github.event_name == 'pull_request' run: cargo test -p bc-forge-admin diff --git a/contracts/admin/src/lib.rs b/contracts/admin/src/lib.rs index ef0bd2a2..2f1ce108 100644 --- a/contracts/admin/src/lib.rs +++ b/contracts/admin/src/lib.rs @@ -245,6 +245,8 @@ pub enum AdminError { /// the requested operation. Distinct from [`AdminError::UnauthorizedRole`], /// which is specific to a role-guard failure. Unauthorized = 20, + /// The target address already holds the role being granted (#768). + RoleAlreadyGranted = 21, } /// Storage keys for the access-control layer. @@ -822,8 +824,9 @@ pub fn has_admin(env: &Env) -> bool { /// /// @notice Grants `role` to `address`. Only a super-admin may call this function. /// @dev Requires the caller to hold the `SuperAdmin` role. Rejects the zero address and -/// unrecognized role variants, then emits `role_grnt`. Granting an already-held role -/// is idempotent: the bitmask is ORed, so no state change occurs beyond the event. +/// unrecognized role variants, then emits `role_grnt`. Granting a role the target +/// already holds fails with [`AdminError::RoleAlreadyGranted`] (#768), so callers +/// never mistake a no-op for a fresh assignment. /// @param env The Soroban environment. /// @param caller The address performing the grant; must be a super-admin. /// @param role The role to grant (one of [`Role::Admin`], [`Role::Minter`], [`Role::SuperAdmin`], [`Role::Pauser`]). @@ -832,6 +835,7 @@ pub fn has_admin(env: &Env) -> bool { /// - [`AdminError::UnauthorizedRole`] — `caller` does not hold the `SuperAdmin` role. /// - [`AdminError::InvalidAddress`] — `address` is the canonical zero address. /// - [`AdminError::InvalidRole`] — `role` is not a recognized variant. +/// - [`AdminError::RoleAlreadyGranted`] — `address` already holds `role`. /// # Events /// Emits `role_grnt` with data `(caller, role, address)`. pub fn grant_role(env: &Env, caller: &Address, role: Role, address: &Address) { @@ -847,8 +851,8 @@ pub fn grant_role(env: &Env, caller: &Address, role: Role, address: &Address) { /// @dev Intentionally private. Callers must perform authorization before delegating here. /// Rejects the zero address. The assignment is a single load / bitwise-OR / /// store on the address's `AdminKey::RoleMask(address)` entry, so a grant -/// never disturbs the address's other roles. Granting an already-held role is -/// idempotent. +/// never disturbs the address's other roles. Granting a role the address +/// already holds panics with [`AdminError::RoleAlreadyGranted`] (#768). /// @param env The Soroban environment. /// @param admin The address recorded as the granting caller in the emitted event. /// @param role The role to assign. @@ -856,6 +860,7 @@ pub fn grant_role(env: &Env, caller: &Address, role: Role, address: &Address) { /// @errors /// - [`AdminError::InvalidAddress`] — `address` is the canonical zero address. /// - [`AdminError::InvalidRole`] — `role` is not a recognized variant. +/// - [`AdminError::RoleAlreadyGranted`] — `address` already holds `role`. /// # Events /// Emits `role_grnt` with data `(admin, role, address)`. fn _grant_role(env: &Env, admin: &Address, role: Role, address: &Address) { @@ -865,6 +870,12 @@ fn _grant_role(env: &Env, admin: &Address, role: Role, address: &Address) { None => soroban_sdk::panic_with_error!(env, AdminError::InvalidRole), }; let mask = load_role_mask(env, address); + // A role that is already held must fail loudly rather than silently + // no-op: callers that rely on the grant having *changed* something would + // otherwise get a false sense of a fresh assignment (#768). + if mask & bit != 0 { + soroban_sdk::panic_with_error!(env, AdminError::RoleAlreadyGranted); + } persist_role_mask(env, address, mask | bit); events::emit_role_granted(env, admin, role, address); } diff --git a/contracts/admin/src/tests/proptest.rs b/contracts/admin/src/tests/proptest.rs index c678ed64..09eb056e 100644 --- a/contracts/admin/src/tests/proptest.rs +++ b/contracts/admin/src/tests/proptest.rs @@ -85,19 +85,26 @@ proptest! { prop_assert!(client.has_role(&role, &holder)); } - /// Fuzz: granting the same role to the same address N times is idempotent. + /// Fuzz: granting the same role to the same address twice fails with + /// `RoleAlreadyGranted`; the first grant always succeeds (#768). #[test] - fn fuzz_grant_role_idempotent(role_idx in 0u32..4, count in 1..20u32) { + fn fuzz_grant_role_already_granted(role_idx in 0u32..4, count in 1..5u32) { let role = role_for_idx(role_idx); let env = Env::default(); let (client, admin) = setup(&env); let holder = Address::generate(&env); + client.grant_role(&admin, &role, &holder); + prop_assert!(client.has_role(&role, &holder)); + + // Every subsequent grant of the same role must fail loudly. for _ in 0..count { - client.grant_role(&admin, &role, &holder); + let result = client.try_grant_role(&admin, &role, &holder); + prop_assert_eq!( + result, + Err(Ok(soroban_sdk::Error::from_contract_error(21))) + ); } - - prop_assert!(client.has_role(&role, &holder)); } /// Fuzz: any subset of roles can be granted to the same address. @@ -192,7 +199,16 @@ proptest! { let super_admin = Address::generate(&env); client.grant_role(&admin, &Role::SuperAdmin, &super_admin); - client.grant_role(&super_admin, &role, &super_admin); + if role == Role::SuperAdmin { + // #768: a role the address already holds cannot be re-granted. + let result = client.try_grant_role(&super_admin, &role, &super_admin); + prop_assert_eq!( + result, + Err(Ok(soroban_sdk::Error::from_contract_error(21))) + ); + } else { + client.grant_role(&super_admin, &role, &super_admin); + } prop_assert!(client.has_role(&role, &super_admin)); } diff --git a/contracts/admin/src/tests/rbac_errors.rs b/contracts/admin/src/tests/rbac_errors.rs index f64f0366..ca802558 100644 --- a/contracts/admin/src/tests/rbac_errors.rs +++ b/contracts/admin/src/tests/rbac_errors.rs @@ -4,6 +4,17 @@ use super::*; use soroban_sdk::InvokeError; +/// Minimal client harness: registers `AdminContract`, sets an admin, and +/// returns the client plus the admin address. +fn setup(env: &Env) -> (AdminContractClient<'_>, Address) { + env.mock_all_auths(); + let contract_id = env.register(AdminContract, ()); + let client = AdminContractClient::new(env, &contract_id); + let admin = Address::generate(env); + client.set_admin(&admin); + (client, admin) +} + /// Asserts that the module-level documentation table and the code-level /// discriminants agree on the standardized PascalCase error names (#751). #[test] @@ -52,7 +63,8 @@ fn test_admin_error_variants_are_pascal_case_and_unique() { check_variant!(ProposalNotPending = 18); check_variant!(DuplicateVote = 19); check_variant!(Unauthorized = 20); - assert_eq!(count, 20, "expected all 20 standardized error variants"); + check_variant!(RoleAlreadyGranted = 21); + assert_eq!(count, 21, "expected all 21 standardized error variants"); } /// The `Unauthorized` variant (#752) must exist and convert into a Soroban @@ -111,6 +123,42 @@ fn test_mask_without_role_bitwise_and_not() { assert_eq!(mask_without_role(cleared, Role::Minter), cleared); } +/// #761 — every recognized role discriminant passes grant_role's validation. +/// The public `Role` type is a `#[contracttype]` enum (name-encoded), so the +/// type system already excludes unknown discriminants; this test locks in that +/// each valid role is accepted end-to-end and mapped to its bitmask bit. +#[test] +fn test_grant_role_accepts_every_recognized_role() { + let env = Env::default(); + env.mock_all_auths(); + let (client, admin) = setup(&env); + + for role in [Role::Admin, Role::Minter, Role::SuperAdmin, Role::Pauser] { + let holder = Address::generate(&env); + client.grant_role(&admin, &role, &holder); + assert!(client.has_role(&role, &holder)); + // Role bit is the power-of-two bound the issue's "valid bitmask" step + // checks (#761): exactly one bit is set for each recognized role. + assert_eq!(role_bit(role), Some(mask_with_role(0, role))); + } +} + +/// #768 — granting a role the target already holds fails with +/// `RoleAlreadyGranted`, not a silent no-op. +#[test] +fn test_grant_role_already_granted_fails() { + let env = Env::default(); + env.mock_all_auths(); + let (client, admin) = setup(&env); + let holder = Address::generate(&env); + + client.grant_role(&admin, &Role::Minter, &holder); + assert!(client.has_role(&Role::Minter, &holder)); + + let result = client.try_grant_role(&admin, &Role::Minter, &holder); + assert_eq!(result, Err(Ok(soroban_sdk::Error::from_contract_error(21)))); +} + /// The four role bits are exactly 1, 2, 4, 8 (#753: bitwise values 1, 2, 4, 8). #[test] fn test_role_bits_are_1_2_4_8() { diff --git a/contracts/lifecycle/test_snapshots/tests/test_non_pauser_cannot_unpause.1.json b/contracts/lifecycle/test_snapshots/tests/test_non_pauser_cannot_unpause.1.json index 89f4bfd6..fd486124 100644 --- a/contracts/lifecycle/test_snapshots/tests/test_non_pauser_cannot_unpause.1.json +++ b/contracts/lifecycle/test_snapshots/tests/test_non_pauser_cannot_unpause.1.json @@ -78,14 +78,7 @@ "key": { "vec": [ { - "symbol": "Role" - }, - { - "vec": [ - { - "symbol": "Admin" - } - ] + "symbol": "RoleMask" }, { "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM" @@ -105,14 +98,7 @@ "key": { "vec": [ { - "symbol": "Role" - }, - { - "vec": [ - { - "symbol": "Admin" - } - ] + "symbol": "RoleMask" }, { "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM" @@ -121,7 +107,7 @@ }, "durability": "persistent", "val": { - "bool": true + "u32": 1 } } }, diff --git a/contracts/lifecycle/test_snapshots/tests/test_pauser_role_can_unpause.1.json b/contracts/lifecycle/test_snapshots/tests/test_pauser_role_can_unpause.1.json index 710957a9..f1276e88 100644 --- a/contracts/lifecycle/test_snapshots/tests/test_pauser_role_can_unpause.1.json +++ b/contracts/lifecycle/test_snapshots/tests/test_pauser_role_can_unpause.1.json @@ -145,14 +145,7 @@ "key": { "vec": [ { - "symbol": "Role" - }, - { - "vec": [ - { - "symbol": "Admin" - } - ] + "symbol": "RoleMask" }, { "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM" @@ -172,14 +165,7 @@ "key": { "vec": [ { - "symbol": "Role" - }, - { - "vec": [ - { - "symbol": "Admin" - } - ] + "symbol": "RoleMask" }, { "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM" @@ -188,7 +174,7 @@ }, "durability": "persistent", "val": { - "bool": true + "u32": 1 } } }, @@ -204,14 +190,7 @@ "key": { "vec": [ { - "symbol": "Role" - }, - { - "vec": [ - { - "symbol": "Pauser" - } - ] + "symbol": "RoleMask" }, { "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M" @@ -231,14 +210,7 @@ "key": { "vec": [ { - "symbol": "Role" - }, - { - "vec": [ - { - "symbol": "Pauser" - } - ] + "symbol": "RoleMask" }, { "address": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAHK3M" @@ -247,7 +219,7 @@ }, "durability": "persistent", "val": { - "bool": true + "u32": 8 } } }, diff --git a/scripts/check_crate_cycles.py b/scripts/check_crate_cycles.py new file mode 100644 index 00000000..e65c93f7 --- /dev/null +++ b/scripts/check_crate_cycles.py @@ -0,0 +1,95 @@ +#!/usr/bin/env python3 +"""Detect circular dependencies between bc-forge workspace crates (#770). + +Cargo resolves the dependency graph, but a cycle between workspace members is +usually a design smell that compiles only by luck of link order and confuses +`cargo tree`. This script walks the declared `path = "../*"` dependencies of +every workspace member and fails if a cycle exists. + +Usage: + python3 scripts/check_crate_cycles.py +""" + +import pathlib +import sys +import re + +ROOT = pathlib.Path(__file__).resolve().parent.parent +WORKSPACE = ROOT / "Cargo.toml" +CONTRACTS = ROOT / "contracts" + +# name -> list of workspace-member names it depends on (via a path dep). +graph = {} + + +def member_names() -> list[str]: + """Workspace members that live under contracts/.""" + names = [] + for toml in CONTRACTS.glob("*/Cargo.toml"): + text = toml.read_text(encoding="utf-8") + name_match = re.search(r'^name\s*=\s*"([^"]+)"', text, re.MULTILINE) + if name_match: + names.append(name_match.group(1)) + return sorted(names) + + +def build_graph() -> None: + for toml in CONTRACTS.glob("*/Cargo.toml"): + text = toml.read_text(encoding="utf-8") + name_match = re.search(r'^name\s*=\s*"([^"]+)"', text, re.MULTILINE) + if not name_match: + continue + name = name_match.group(1) + deps = set() + for dep_match in re.finditer(r'^\s*([\w-]+)\s*=\s*\{\s*path\s*=\s*"\.\./', text, re.MULTILINE): + deps.add(dep_match.group(1)) + graph[name] = deps + + +def find_cycle() -> list[str] | None: + WHITE, GRAY, BLACK = 0, 1, 2 + color = {n: WHITE for n in graph} + stack = [] + + def visit(node: str) -> list[str] | None: + color[node] = GRAY + stack.append(node) + for dep in graph.get(node, ()): + if dep not in graph: + continue # external or non-member path dep + if color[dep] == GRAY: + cycle_start = stack.index(dep) + return stack[cycle_start:] + [dep] + if color[dep] == WHITE: + cycle = visit(dep) + if cycle: + return cycle + stack.pop() + color[node] = BLACK + return None + + for node in sorted(graph): + if color[node] == WHITE: + cycle = visit(node) + if cycle: + return cycle + return None + + +def main() -> int: + if not WORKSPACE.exists(): + print("error: expected a Cargo workspace at the repo root", file=sys.stderr) + return 2 + + build_graph() + cycle = find_cycle() + if cycle: + print("error: circular dependency detected:", " -> ".join(cycle)) + return 1 + + print(f"ok: {len(graph)} workspace crates have no circular path dependencies") + return 0 + + +if __name__ == "__main__": + sys.exit(main())