[N-03] Access Control Audit

[ericnordelo]

Summary by CodeRabbit

• Breaking Changes

  • Updated renounce_role signature: the account parameter has been removed. Callers must now rely on the transaction context instead of specifying an explicit account.

• New Features

  • Enhanced new_with_admin constructor to properly initialize with the provided initial administrator address and emit corresponding events.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f3596fb1-8918-420a-bbb5-54b378ea426f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/access-control-N-04

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.93%. Comparing base (aff58cb) to head (7b15d59).

Additional details and impacted files

@@               Coverage Diff                @@
##           release-v1.2     #337      +/-   ##
================================================
+ Coverage         95.88%   95.93%   +0.04%     
================================================
  Files                22       22              
  Lines              2187     2187              
  Branches            592      593       +1     
================================================
+ Hits               2097     2098       +1     
  Misses               65       65              
+ Partials             25       24       -1     

Flag	Coverage Δ
contracts/access	64.25% <100.00%> (+0.15%)	⬆️
math/core	86.12% <ø> (ø)
math/fixed_point	55.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR remediates access-control audit feedback by tightening role storage typing, adding an explicit initial-admin constructor, and simplifying role renunciation to always apply to the transaction sender.

Changes:

• Replaces untyped Bag role storage with Table<TypeName, RoleData>.
• Adds new_with_admin and refactors new to delegate to it.
• Removes the explicit account parameter from renounce_role and updates tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
contracts/access/sources/access_control.move	Updates storage, constructors, renounce API, error constants, and role accessors.
contracts/access/tests/access_control_tests.move	Adds constructor tests and updates renounce tests for the new signature.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

contracts/access/sources/access_control.move (1)

161-165: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Upgrade safety: AccessControl.roles layout change is breaking for published key objects
In Sui package upgrades, changing the field type of an existing public key struct (here roles: Bag → roles: Table<TypeName, RoleData>) is not upgrade-safe because struct layouts must remain strictly identical for previously published objects. A release that mutates this struct in place will require a migration/versioning approach instead (e.g., keep the old struct layout, add AccessControlV2 with the new field type, and provide an explicit migration/swap entry function, or use a versioned wrapper/dynamic-field migration pattern).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/access/sources/access_control.move` around lines 161 - 165, The
change to AccessControl<RootRole>::roles from Bag to Table breaks upgrade safety
for published key objects; revert the in-place field-type change and instead
preserve the original struct layout by keeping the existing AccessControl struct
unchanged and introduce a new AccessControlV2 (or similar) that has roles:
Table<TypeName, RoleData>; add a migration entry function (e.g.,
migrate_access_control_to_v2) that accepts the old AccessControl key object,
performs a controlled in-contract migration/swap to construct AccessControlV2
(moving data from the old Bag into the new Table), and enforces proper access
checks/versions; alternatively add a new field (e.g., new_roles: Table<...>) to
the existing struct while keeping the old roles field to preserve layout and
provide an explicit migration function to populate and then retire the old
field. Ensure all references use the new type names (AccessControl,
AccessControlV2, roles, new_roles, migrate_access_control_to_v2) so upgrades are
safe.

🧹 Nitpick comments (1)

contracts/access/tests/access_control_tests.move (1)

457-459: ⚡ Quick win

Update the stale storage wording.

This comment still describes the old Bag-backed layout, but the code under test now stores roles in a Table<TypeName, RoleData>. Keeping the branch description in sync with the implementation will make these renounce idempotency cases easier to follow.

♻️ Proposed wording update

-// Renounce idempotency: role in bag, but the caller is not a member — the
-// second early-return path. Distinct from the "role not in bag" path covered
+// Renounce idempotency: role exists in the table, but the caller is not a
+// member — the second early-return path. Distinct from the "role not in the
+// table" path covered

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/access/tests/access_control_tests.move` around lines 457 - 459,
Update the test comment describing the "renounce idempotency" case to reflect
the current Table-backed storage: replace references to "bag" with
"Table<TypeName, RoleData>" and reword the branch description to say that the
role exists in the Table but the caller is not a member (the second early-return
path), so the comment matches the implementation under test (roles stored in
Table<TypeName, RoleData>).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@contracts/access/sources/access_control.move`:
- Around line 83-115: The abort-code numbers were shifted when removing
ECannotRenounceForOtherAccount, which breaks callers/tests that depend on
numeric codes; restore the original numeric slot by reintroducing a placeholder
error constant named ECannotRenounceForOtherAccount (or a clearly marked
reserved constant) with the original #[error(code = 4)] annotation and a comment
marking it retired/unused, and keep all subsequent constants (EDelayTooLarge,
ENotOneTimeWitness, EForeignRole, ENotPendingTransfer, ENotPendingRenounce,
ENoPendingDelayChange, EZeroAddress, EDefaultAdminTransferToSelf) using their
existing #[error(code = ...)] values so their numeric codes do not change.
Ensure the placeholder is unused in logic but present in
contracts/access/sources/access_control.move to preserve the API.

---

Outside diff comments:
In `@contracts/access/sources/access_control.move`:
- Around line 161-165: The change to AccessControl<RootRole>::roles from Bag to
Table breaks upgrade safety for published key objects; revert the in-place
field-type change and instead preserve the original struct layout by keeping the
existing AccessControl struct unchanged and introduce a new AccessControlV2 (or
similar) that has roles: Table<TypeName, RoleData>; add a migration entry
function (e.g., migrate_access_control_to_v2) that accepts the old AccessControl
key object, performs a controlled in-contract migration/swap to construct
AccessControlV2 (moving data from the old Bag into the new Table), and enforces
proper access checks/versions; alternatively add a new field (e.g., new_roles:
Table<...>) to the existing struct while keeping the old roles field to preserve
layout and provide an explicit migration function to populate and then retire
the old field. Ensure all references use the new type names (AccessControl,
AccessControlV2, roles, new_roles, migrate_access_control_to_v2) so upgrades are
safe.

---

Nitpick comments:
In `@contracts/access/tests/access_control_tests.move`:
- Around line 457-459: Update the test comment describing the "renounce
idempotency" case to reflect the current Table-backed storage: replace
references to "bag" with "Table<TypeName, RoleData>" and reword the branch
description to say that the role exists in the Table but the caller is not a
member (the second early-return path), so the comment matches the implementation
under test (roles stored in Table<TypeName, RoleData>).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d02053ba-2a36-485f-a25b-4cc483386d61

📥 Commits

Reviewing files that changed from the base of the PR and between d481f4a and 4bec799.

📒 Files selected for processing (2)

• contracts/access/sources/access_control.move
• contracts/access/tests/access_control_tests.move

[bidzyyys]

Looks good to me.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[0xNeshi]

🔥