[N-02] Access Control Audit

[ericnordelo]

Summary by CodeRabbit

• New Features

  • Implemented validation preventing the current default admin from initiating a role transfer to themselves, strengthening access control security.
  • Clarified documentation on authentication witnesses to confirm they remain valid throughout transaction execution despite subsequent role changes.

• Tests

  • Added test coverage verifying rejection of default admin self-transfers.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 928d540f-aca9-43d5-bda8-10e266cf2d8e

📥 Commits

Reviewing files that changed from the base of the PR and between 9cd0672 and e79fa8e.

📒 Files selected for processing (2)

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

---

Walkthrough

The access control contract now prevents a root holder from initiating their own role transfer by rejecting when the new admin matches the transaction sender. A new error constant is defined, a runtime assertion added, documentation updated to clarify PTB-local witness snapshots, and tests adjusted to cover the new rejection case while removing the prior self-transfer acceptance test.

Changes

Access Control Admin Transfer Self-Prevention

Layer / File(s)	Summary
Self-transfer prevention error and assertion contracts/access/sources/access_control.move	New error constant EDefaultAdminTransferToSelf (code 13) is introduced. The begin_default_admin_transfer function adds a runtime assertion rejecting new_admin == ctx.sender(), and function documentation is extended to include the new abort condition.
PTB-local witness snapshot documentation contracts/access/sources/access_control.move	Documentation for Auth and new_auth is updated to clarify that the witness is a PTB-local snapshot whose validity is decoupled from any later same-PTB renunciation effects.
Test coverage for self-transfer prevention contracts/access/tests/access_control_tests.move	New negative test test_begin_admin_transfer_rejects_self verifies that begin_default_admin_transfer aborts with EDefaultAdminTransferToSelf when the new admin equals the current root holder. The prior test_accept_admin_transfer_to_self test, which exercised the now-prevented self-transfer acceptance path, is removed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• OpenZeppelin/contracts-sui#335: Both PRs add the same EDefaultAdminTransferToSelf check in begin_default_admin_transfer and update access_control_tests.move by adding the same self-transfer rejection negative test while removing the prior self-transfer acceptance test.

Suggested reviewers

• immrsd
• 0xNeshi
• bidzyyys

Poem

🐰 No transferring to thyself, we say,
A transfer rule to save the day!
The root must find a new one true,
Not mirror back their own review.
Tests now guard this logic tight, 🔐

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided by the author; required sections from the template (issue reference, change description, and PR checklist) are missing.	Add a description that references the related issue, explains the changes (default admin self-transfer prevention), and completes the PR checklist items.
Title check	❓ Inconclusive	The title references an audit finding (N-02) but lacks specificity about what access control change is being implemented.	Use a more descriptive title that clearly states the specific change, such as 'Prevent self-transfer of default admin role' or 'Add validation to reject self-transfers in access control'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

---

_{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.89%. Comparing base (d481f4a) to head (e79fa8e).

Additional details and impacted files

@@               Coverage Diff                @@
##           release-v1.2     #336      +/-   ##
================================================
+ Coverage         95.88%   95.89%   +0.01%     
================================================
  Files                22       23       +1     
  Lines              2187     2364     +177     
  Branches            593      622      +29     
================================================
+ Hits               2097     2267     +170     
  Misses               65       65              
- Partials             25       32       +7     

Flag	Coverage Δ
contracts/access	64.09% <ø> (ø)
contracts/utils	42.28% <ø> (?)
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.

[bidzyyys]

LGTM