feat(rbac): reject duplicate role grants, validate roles, and add dependency check (#761 #768 #770)

.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

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,15 +851,16 @@ 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.
 /// @param address The address to receive the role.
 /// @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);
 }

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));
     }
 

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() {

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
                 }
               }
             },

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
                 }
               }
             },