feat(access-manager): simple reentrancy test

benluelo · benluelo · commit ec1e5cf6f635 · 2025-10-06T10:52:38.000+01:00
diff --git a/cosmwasm/access-manager/src/contract.rs b/cosmwasm/access-manager/src/contract.rs
@@ -36,6 +36,7 @@ use crate::{
 /// ```
 ///
 /// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L123>
+#[inline]
 fn only_authorized(ctx: &mut ExecCtx) -> Result<(), ContractError> {
     _check_authorized(ctx)
 }
@@ -144,7 +145,7 @@ pub(crate) fn set_grant_delay(
 /// ) internal virtual returns (bool)
 /// ```
 ///
-/// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L270-L275>
+/// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L270>
 pub(crate) fn _grant_role(
     ctx: &mut ExecCtx,
     role_id: RoleId,
@@ -446,7 +447,7 @@ pub(crate) fn set_target_closed(
 /// function _setTargetClosed(address target, bool closed) internal virtual
 /// ```
 ///
-/// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L422C5-L422C76>
+/// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L422>
 fn _set_target_closed(ctx: &mut ExecCtx, target: &Addr, closed: bool) -> Result<(), ContractError> {
     ctx.storage()
         .upsert::<Targets, ContractError>(target, |maybe_target_config| {
@@ -496,11 +497,13 @@ pub(crate) fn schedule(
 ) -> Result<(H256, u32), ContractError> {
     let caller = ctx.msg_sender();
 
+    dbg!(data);
+
     // Fetch restrictions that apply to the caller on the targeted function
     let CanCall {
         allowed: _,
         delay: setback,
-    } = _can_call_extended(ctx, caller, target, data)?;
+    } = dbg!(_can_call_extended(ctx, caller, target, data)?);
 
     let min_when = ctx.timestamp() + u64::from(setback);
 
@@ -769,6 +772,7 @@ pub(crate) fn update_authority(
 /// ```
 ///
 /// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L598>
+#[inline]
 fn _check_authorized(ctx: &mut ExecCtx) -> Result<(), ContractError> {
     let msg_data = ctx.msg_data();
 
@@ -811,7 +815,7 @@ fn _check_authorized(ctx: &mut ExecCtx) -> Result<(), ContractError> {
 /// ) private view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay)
 /// ```
 ///
-/// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L619-L621>
+/// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L619>
 fn _get_admin_restrictions(
     ctx: &mut ExecCtx,
     data: &str,
@@ -840,9 +844,11 @@ fn _get_admin_restrictions(
 
         // Restricted to that role's admin with no delay beside any execution delay the caller may
         // have.
-        Ok(GrantRole { role_id, .. } | RevokeRole { role_id, .. }) => {
-            Ok((true, get_role_admin(ctx.query_ctx(), role_id)?, 0))
-        }
+        Ok(GrantRole { role_id, .. } | RevokeRole { role_id, .. }) => Ok((
+            true,
+            dbg!(get_role_admin(ctx.query_ctx(), dbg!(role_id))?),
+            0,
+        )),
 
         _ => Ok((
             false,
@@ -890,7 +896,7 @@ fn _can_call_extended(
     data: &str,
 ) -> Result<CanCall, ContractError> {
     if target == ctx.address_this() {
-        _can_call_self(ctx, caller, data)
+        dbg!(_can_call_self(ctx, caller, data))
     } else {
         can_call(ctx.query_ctx(), caller, target, _check_selector(data)?)
     }
@@ -905,7 +911,7 @@ fn _can_call_extended(
 /// <https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.4.0/contracts/access/manager/AccessManager.sol#L685>
 fn _can_call_self(ctx: &mut ExecCtx, caller: &Addr, data: &str) -> Result<CanCall, ContractError> {
     if caller == ctx.address_this() {
-        // Caller is AccessManager, this means the call was sent through {execute} and it already
+        // Caller is AccessManager, this means the call was sent through `execute` and it already
         // checked permissions. We verify that the call "identifier", which is set during
         // `execute`, is correct.
         return Ok(CanCall {
@@ -916,6 +922,8 @@ fn _can_call_self(ctx: &mut ExecCtx, caller: &Addr, data: &str) -> Result<CanCal
 
     let (admin_restricted, role_id, operation_delay) = _get_admin_restrictions(ctx, data)?;
 
+    dbg!((admin_restricted, role_id, operation_delay));
+
     // isTargetClosed apply to non-admin-restricted function
     if !admin_restricted && is_target_closed(ctx.query_ctx(), ctx.address_this())? {
         return Ok(CanCall {
@@ -927,7 +935,7 @@ fn _can_call_self(ctx: &mut ExecCtx, caller: &Addr, data: &str) -> Result<CanCal
     let HasRole {
         is_member,
         execution_delay,
-    } = has_role(ctx.query_ctx(), role_id, caller)?;
+    } = dbg!(has_role(ctx.query_ctx(), role_id, caller)?);
 
     if !is_member {
         return Ok(CanCall {
diff --git a/cosmwasm/access-manager/src/lib.rs b/cosmwasm/access-manager/src/lib.rs
@@ -15,7 +15,8 @@
 //!
 //! For each target contract, admins can configure the following without any delay:
 //!
-//! - The target's {AccessManaged-authority} via [`ExecuteMsg::UpdateAuthority`].
+//! - The target's [`QueryMsg::Authority`][access_manager_types::managed::msg::QueryMsg::Authority]
+//!   via [`ExecuteMsg::UpdateAuthority`].
 //! - Close or open a target via [`ExecuteMsg::SetTargetClosed`] keeping the permissions intact.
 //! - The roles that are allowed (or disallowed) to call a given function (identified by its
 //!   selector) through [`ExecuteMsg::SetTargetAdminDelay`].
diff --git a/cosmwasm/access-manager/src/tests.rs b/cosmwasm/access-manager/src/tests.rs
@@ -1408,3 +1408,100 @@ fn schedule_works() {
         }),
     );
 }
+
+#[test]
+fn schedule_reentrant_works() {
+    const GRANT_ROLE: RoleId = RoleId::new(6);
+
+    let (mut deps, env) = setup();
+
+    execute(
+        deps.as_mut(),
+        env.clone(),
+        message_info(&ADMIN, &[]),
+        ExecuteMsg::SetTargetFunctionRole {
+            target: env.contract.address.clone(),
+            selectors: vec![Selector::new("grant_role").to_owned()],
+            role_id: GRANT_ROLE,
+        },
+    )
+    .unwrap();
+
+    execute(
+        deps.as_mut(),
+        env.clone(),
+        message_info(&ADMIN, &[]),
+        ExecuteMsg::GrantRole {
+            account: ACCOUNT_1.clone(),
+            role_id: GRANT_ROLE,
+            execution_delay: 10,
+        },
+    )
+    .unwrap();
+
+    execute(
+        deps.as_mut(),
+        env.clone(),
+        message_info(&ADMIN, &[]),
+        ExecuteMsg::GrantRole {
+            account: ACCOUNT_1.clone(),
+            role_id: RoleId::new(11),
+            execution_delay: 20,
+        },
+    )
+    .unwrap();
+
+    execute(
+        deps.as_mut(),
+        env.clone(),
+        message_info(&ADMIN, &[]),
+        ExecuteMsg::SetRoleAdmin {
+            role_id: RoleId::new(10),
+            admin: RoleId::new(11),
+        },
+    )
+    .unwrap();
+
+    execute(
+        deps.as_mut(),
+        env.clone(),
+        message_info(&ADMIN, &[]),
+        ExecuteMsg::SetTargetFunctionRole {
+            role_id: GRANT_ROLE,
+            target: env.contract.address.clone(),
+            selectors: vec![Selector::new("grant_role").to_owned()],
+        },
+    )
+    .unwrap();
+
+    eprintln!("NOW\n\n\n");
+
+    assert_eq!(
+        execute(
+            deps.as_mut(),
+            env.clone(),
+            message_info(&ACCOUNT_1, &[]),
+            ExecuteMsg::Schedule {
+                target: env.contract.address.clone(),
+                data: serde_json::to_string(&ExecuteMsg::GrantRole {
+                    role_id: RoleId::new(10),
+                    account: ACCOUNT_2.clone(),
+                    execution_delay: 0
+                })
+                .unwrap(),
+                when: env.block.time.seconds() + 20
+            },
+        )
+        .unwrap(),
+        Response::new().add_event(OperationScheduled {
+            operation_id: H256::new(hex!(
+                "8851fd1669d010b077f22bf956ea2ae240fe964d0f9d30e46f702b6e950278b5"
+            )),
+            nonce: 1,
+            schedule: env.block.time.seconds() + 20,
+            caller: &ACCOUNT_1,
+            target: &env.contract.address,
+            data: r#"{"grant_role":{"role_id":"10","account":"account-2","execution_delay":0}}"#,
+        }),
+    );
+}
diff --git a/e2e/access-manager-tests/src/main.rs b/e2e/access-manager-tests/src/main.rs
@@ -56,6 +56,9 @@ const DECREMENT: RoleId = RoleId::new(2);
 const INCREMENT_GUARDIAN: RoleId = RoleId::new(4);
 const DECREMENT_GUARDIAN: RoleId = RoleId::new(5);
 
+const GRANT_ROLE: RoleId = RoleId::new(6);
+const GRANT_ROLE_ADMIN: RoleId = RoleId::new(7);
+
 fn mk_wallet(mnemonic: &str, bech32_prefix: &str) -> LocalSigner {
     LocalSigner::new(
         tiny_hderive::bip32::ExtendedPrivKey::derive(
@@ -220,11 +223,13 @@ async fn main() -> Result<()> {
     )
     .await?;
 
-    schedule_increment(&alice_client, &bob_client, &manager, &managed).await?;
+    // schedule_increment(&alice_client, &bob_client, &manager, &managed).await?;
+
+    // schedule_decrement_in_sub_msg(&alice_client, &bob_client, &manager, &managed).await?;
 
-    schedule_decrement_in_sub_msg(&alice_client, &bob_client, &manager, &managed).await?;
+    // schedule_increment_in_reply(&alice_client, &bob_client, &manager, &managed).await?;
 
-    schedule_increment_in_reply(&alice_client, &bob_client, &manager, &managed).await?;
+    schedule_reentrant(&alice_client, &bob_client, &charlie_client, &manager).await?;
 
     Ok(())
 }
@@ -544,6 +549,144 @@ async fn schedule_increment_in_reply(
     Ok(())
 }
 
+#[instrument(skip_all)]
+async fn schedule_reentrant(
+    alice_client: &TxClient<impl WalletT, impl RpcT, impl GasFillerT>,
+    bob_client: &TxClient<impl WalletT, impl RpcT, impl GasFillerT>,
+    charlie_client: &TxClient<impl WalletT, impl RpcT, impl GasFillerT>,
+    manager: &Bech32<H256>,
+) -> Result<()> {
+    execute(
+        alice_client,
+        manager,
+        [
+            &manager::msg::ExecuteMsg::SetTargetFunctionRole {
+                target: Addr::unchecked(manager.to_string()),
+                selectors: vec![Selector::new("grant_role").to_owned()],
+                role_id: GRANT_ROLE,
+            },
+            &manager::msg::ExecuteMsg::GrantRole {
+                account: Addr::unchecked(charlie_client.wallet().address().to_string()),
+                role_id: GRANT_ROLE,
+                execution_delay: 10,
+            },
+            &manager::msg::ExecuteMsg::GrantRole {
+                account: Addr::unchecked(charlie_client.wallet().address().to_string()),
+                role_id: RoleId::new(11),
+                execution_delay: 20,
+            },
+            &manager::msg::ExecuteMsg::SetRoleAdmin {
+                role_id: RoleId::new(10),
+                admin: RoleId::new(11),
+            },
+            &manager::msg::ExecuteMsg::SetTargetFunctionRole {
+                role_id: GRANT_ROLE,
+                target: Addr::unchecked(manager.to_string()),
+                selectors: vec![Selector::new("grant_role").to_owned()],
+            },
+        ],
+    )
+    .await?;
+
+    // let msg = manager::msg::ExecuteMsg::Schedule {
+    //     target: Addr::unchecked(manager.to_string()),
+    //     data: serde_json::to_string(&manager::msg::ExecuteMsg::GrantRole {
+    //         account: Addr::unchecked(bob_client.wallet().address().to_string()),
+    //         role_id: RoleId::new(7),
+    //         execution_delay: 10,
+    //     })
+    //     .unwrap(),
+    //     when: now() + 10,
+    // };
+    let msg = manager::msg::ExecuteMsg::GrantRole {
+        role_id: RoleId::new(10),
+        account: Addr::unchecked(bob_client.wallet().address().to_string()),
+        execution_delay: 0,
+    };
+    let data = to_json_string(&msg).unwrap();
+    let operation_id = hash_operation(charlie_client.wallet().address(), manager, &msg);
+
+    info!(
+        "charlie can't call grant_role since they have an execution delay, they must schedule the call"
+    );
+    execute_expect_error(
+        charlie_client,
+        manager,
+        &msg,
+        AccessManagerError::AccessManagerNotScheduled(operation_id),
+    )
+    .await?;
+
+    info!("schedule grant_role call too soon");
+    execute_expect_error(
+        charlie_client,
+        manager,
+        &manager::msg::ExecuteMsg::Schedule {
+            target: Addr::unchecked(manager.to_string()),
+            data: data.clone(),
+            when: now() + 5,
+        },
+        AccessManagerError::AccessManagerUnauthorizedCall {
+            caller: Addr::unchecked(charlie_client.wallet().address().to_string()),
+            target: Addr::unchecked(manager.to_string()),
+            selector: Selector::new("grant_role").to_owned(),
+        },
+    )
+    .await?;
+
+    info!("schedule grant_role call with correct delay");
+    let when = now() + 25;
+    execute(
+        charlie_client,
+        manager,
+        [manager::msg::ExecuteMsg::Schedule {
+            target: Addr::unchecked(manager.to_string()),
+            data: data.clone(),
+            when,
+        }],
+    )
+    .await?;
+
+    info!("execute scheduled call too soon");
+    execute_expect_error(
+        charlie_client,
+        manager,
+        &manager::msg::ExecuteMsg::Execute {
+            target: Addr::unchecked(manager.to_string()),
+            data: data.clone(),
+        },
+        AccessManagerError::AccessManagerNotReady(operation_id),
+    )
+    .await?;
+
+    info!("scheduled op not ready");
+    execute_expect_error(
+        charlie_client,
+        manager,
+        &msg,
+        AccessManagerError::AccessManagerNotReady(operation_id),
+    )
+    .await?;
+
+    wait_for_finalized_block_with(alice_client.rpc().client(), |block| {
+        block.block.header.time.seconds.inner() as u64 > when
+    })
+    .await?;
+
+    info!("execute scheduled call once ready");
+    execute(
+        charlie_client,
+        manager,
+        [manager::msg::ExecuteMsg::Execute {
+            target: Addr::unchecked(manager.to_string()),
+            data: data.clone(),
+        }],
+    )
+    .await?;
+
+    Ok(())
+}
+
 fn now() -> u64 {
     std::time::SystemTime::now()
         .duration_since(std::time::UNIX_EPOCH)
@@ -668,6 +811,7 @@ async fn execute(
     Ok(())
 }
 
+// #[track_caller]
 async fn execute_expect_error(
     signer: &TxClient<impl WalletT, impl RpcT, impl GasFillerT>,
     contract: &Bech32<H256>,
diff --git a/lib/access-manager-types/src/managed/msg.rs b/lib/access-manager-types/src/managed/msg.rs
diff --git a/lib/access-manager-types/src/manager/msg.rs b/lib/access-manager-types/src/manager/msg.rs
