Skip to content

[N-01] Access Control Audit #339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ericnordelo merged 8 commits into from
Jun 1, 2026
Merged

[N-01] Access Control Audit #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
90648c3
feat: update delay logic
ericnordelo May 29, 2026
56d59c1
feat: improve doc comments
ericnordelo May 29, 2026
bec0fdf
feat: improve events
ericnordelo May 29, 2026
b9e215c
feat: apply review update
ericnordelo Jun 1, 2026
b2da205
Merge branch 'release-v1.2' of github.com:OpenZeppelin/contracts-sui …
ericnordelo Jun 1, 2026
1c373c2
feat: apply review updates
ericnordelo Jun 1, 2026
ebc349c
Merge branch 'fix/access-control-N-05' into fix/access-control-N-001
ericnordelo Jun 1, 2026
30672bc
Merge branch 'release-v1.2' of github.com:OpenZeppelin/contracts-sui …
ericnordelo Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 37 additions & 21 deletions contracts/access/sources/access_control.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,12 @@ public struct PendingDelayChange has drop, store {

// === Events ===

/// Access-control events are parameterized by `RootRole` so the event type
/// identifies the registry without duplicating registry fields in every
/// payload. Event attribution is available from the Sui event envelope
/// (`SuiEvent.sender`) and is intentionally not duplicated in the Move event
/// payloads.
///
// Access-control events are parameterized by `RootRole` so the event type
// identifies the registry without duplicating registry fields in every
// payload. Event attribution is available from the Sui event envelope
// (`SuiEvent.sender`) and is intentionally not duplicated in the Move event
// payloads.

/// Emitted when a role is granted to an account.
///
/// Role identifiers are `TypeName`s — they embed the defining package address
Expand Down Expand Up @@ -275,12 +275,14 @@ public struct DefaultAdminRenounceScheduled<phantom RootRole> has copy, drop {
execute_after_ms: u64,
}

/// Emitted when a pending root role transfer or renounce is cancelled, either
/// explicitly or because it is overwritten by a new pending action. Indexers
/// can correlate with the prior `DefaultAdminTransferScheduled` or
/// `DefaultAdminRenounceScheduled` event to know which kind was cancelled.
/// Emitted when a pending root role transfer is cancelled, either explicitly or
/// because it is overwritten by a new pending action.
public struct DefaultAdminTransferCancelled<phantom RootRole> has copy, drop {}

/// Emitted when a pending root role renounce is cancelled, either explicitly or
/// because it is overwritten by a new pending action.
public struct DefaultAdminRenounceCancelled<phantom RootRole> has copy, drop {}

/// Emitted when a change to `default_admin_delay_ms` is scheduled.
public struct DefaultAdminDelayChangeScheduled<phantom RootRole> has copy, drop {
new_delay_ms: u64,
Expand Down Expand Up @@ -613,9 +615,7 @@ public fun begin_default_admin_transfer<RootRole>(
refresh_default_admin_delay(ac, clock);
Comment thread
ericnordelo marked this conversation as resolved.

let execute_after_ms = clock.timestamp_ms() + ac.default_admin_delay_ms;
if (ac.pending_default_admin.is_some()) {
event::emit(DefaultAdminTransferCancelled<RootRole> {});
};
cancel_pending_default_admin(ac);
ac.pending_default_admin =
option::some(PendingAdminTransfer {
new_admin: option::some(new_admin),
Expand Down Expand Up @@ -696,9 +696,7 @@ public fun begin_default_admin_renounce<RootRole>(
refresh_default_admin_delay(ac, clock);

let execute_after_ms = clock.timestamp_ms() + ac.default_admin_delay_ms;
if (ac.pending_default_admin.is_some()) {
event::emit(DefaultAdminTransferCancelled<RootRole> {});
};
cancel_pending_default_admin(ac);
ac.pending_default_admin =
option::some(PendingAdminTransfer {
new_admin: option::none(),
Expand Down Expand Up @@ -747,8 +745,8 @@ public fun accept_default_admin_renounce<RootRole>(
}

/// Cancel a pending root role transfer or renounce. Caller must hold the
/// root role. The same function clears either kind; off-chain consumers
/// correlate with the prior schedule event.
/// root role. Emits `DefaultAdminTransferCancelled` or
/// `DefaultAdminRenounceCancelled` according to the pending action kind.
///
/// #### Parameters
/// - `ac`: the registry to mutate.
Expand All @@ -764,9 +762,7 @@ public fun cancel_default_admin_transfer<RootRole>(
assert!(ac.pending_default_admin.is_some(), ENoPendingAdminTransfer);
assert!(ac.has_role_by_name(ac.protected_root, ctx.sender()), EUnauthorized);

let _ = ac.pending_default_admin.extract();

event::emit(DefaultAdminTransferCancelled<RootRole> {});
cancel_pending_default_admin(ac);
}

// === Default Admin Delay Change ===
Expand Down Expand Up @@ -1087,6 +1083,19 @@ fun refresh_default_admin_delay<RootRole>(ac: &mut AccessControl<RootRole>, cloc
true
}

/// Clears a pending root role action, emitting the cancellation event that
/// matches the action kind.
fun cancel_pending_default_admin<RootRole>(ac: &mut AccessControl<RootRole>) {
if (ac.pending_default_admin.is_none()) return;

let pending = ac.pending_default_admin.extract();
if (pending.new_admin.is_some()) {
event::emit(DefaultAdminTransferCancelled<RootRole> {});
} else {
event::emit(DefaultAdminRenounceCancelled<RootRole> {});
};
}

/// Membership check by `TypeName` — used internally when the admin role is
/// known only as a `TypeName` value, not as a type parameter. Root role
/// membership is stored separately as the current default admin.
Expand Down Expand Up @@ -1167,6 +1176,13 @@ public fun test_new_default_admin_transfer_cancelled<RootRole>(): DefaultAdminTr
DefaultAdminTransferCancelled<RootRole> {}
}

#[test_only]
public fun test_new_default_admin_renounce_cancelled<RootRole>(): DefaultAdminRenounceCancelled<
RootRole,
> {
DefaultAdminRenounceCancelled<RootRole> {}
}

#[test_only]
public fun test_new_default_admin_renounce_scheduled<RootRole>(
execute_after_ms: u64,
Expand Down
47 changes: 39 additions & 8 deletions contracts/access/tests/access_control_tests.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,6 +906,12 @@ fun test_begin_admin_transfer_overwrites_pending() {
ACCESS_CONTROL_TESTS,
>();
assert_eq!(cancelled[0], expected);
assert_eq!(
event::events_by_type<
access_control::DefaultAdminRenounceCancelled<ACCESS_CONTROL_TESTS>,
>().length(),
0,
);

clock::destroy_for_testing(clk);
test_scenario::return_shared(ac);
Expand Down Expand Up @@ -1160,9 +1166,8 @@ fun test_cancel_admin_transfer_rejects_non_root() {
abort 999
}

// `cancel_default_admin_transfer` clears either kind of pending action —
// the same function and same `DefaultAdminTransferCancelled` event are
// reused for both transfer and renounce cancels.
// `cancel_default_admin_transfer` clears either kind of pending action and
// emits the cancellation event matching the pending action kind.
#[test]
fun test_cancel_admin_transfer_clears_pending_renounce() {
let deployer = @0xA;
Expand All @@ -1175,12 +1180,20 @@ fun test_cancel_admin_transfer_clears_pending_renounce() {

assert!(!ac.has_pending_default_admin_transfer());
assert!(!ac.is_pending_default_admin_renounce());
// Indexers correlate the cancel event with the prior schedule event to
// know which kind was cleared.
let cancelled = event::events_by_type<
access_control::DefaultAdminTransferCancelled<ACCESS_CONTROL_TESTS>,
access_control::DefaultAdminRenounceCancelled<ACCESS_CONTROL_TESTS>,
>();
assert_eq!(cancelled.length(), 1);
let expected = access_control::test_new_default_admin_renounce_cancelled<
ACCESS_CONTROL_TESTS,
>();
assert_eq!(cancelled[0], expected);
assert_eq!(
event::events_by_type<
access_control::DefaultAdminTransferCancelled<ACCESS_CONTROL_TESTS>,
>().length(),
0,
);

clock::destroy_for_testing(clk);
test_scenario::return_shared(ac);
Expand Down Expand Up @@ -1375,6 +1388,12 @@ fun test_begin_admin_renounce_overwrites_pending_transfer() {
access_control::DefaultAdminTransferCancelled<ACCESS_CONTROL_TESTS>,
>();
assert_eq!(cancelled.length(), 1);
assert_eq!(
event::events_by_type<
access_control::DefaultAdminRenounceCancelled<ACCESS_CONTROL_TESTS>,
>().length(),
0,
);

clock::destroy_for_testing(clk);
test_scenario::return_shared(ac);
Expand All @@ -1399,9 +1418,15 @@ fun test_begin_admin_transfer_overwrites_pending_renounce() {
assert_eq!(ac.pending_default_admin_new_admin(), option::some(new_admin));

let cancelled = event::events_by_type<
access_control::DefaultAdminTransferCancelled<ACCESS_CONTROL_TESTS>,
access_control::DefaultAdminRenounceCancelled<ACCESS_CONTROL_TESTS>,
>();
assert_eq!(cancelled.length(), 1);
assert_eq!(
event::events_by_type<
access_control::DefaultAdminTransferCancelled<ACCESS_CONTROL_TESTS>,
>().length(),
0,
);

clock::destroy_for_testing(clk);
test_scenario::return_shared(ac);
Expand Down Expand Up @@ -1429,9 +1454,15 @@ fun test_begin_admin_renounce_overwrites_pending_renounce() {
assert_eq!(ac.pending_default_admin_execute_after_ms(), option::some(50));

let cancelled = event::events_by_type<
access_control::DefaultAdminTransferCancelled<ACCESS_CONTROL_TESTS>,
access_control::DefaultAdminRenounceCancelled<ACCESS_CONTROL_TESTS>,
>();
assert_eq!(cancelled.length(), 1);
assert_eq!(
event::events_by_type<
access_control::DefaultAdminTransferCancelled<ACCESS_CONTROL_TESTS>,
>().length(),
0,
);

clock::destroy_for_testing(clk);
test_scenario::return_shared(ac);
Expand Down
Loading