Skip to content

Fix audit-trail Move security audit findings#261

Open
itsyaasir wants to merge 9 commits into
feat/audit-trails-devfrom
chore/security-audit
Open

Fix audit-trail Move security audit findings#261
itsyaasir wants to merge 9 commits into
feat/audit-trails-devfrom
chore/security-audit

Conversation

@itsyaasir
Copy link
Copy Markdown
Contributor

Description of change

Fixes two audit-trail Move security audit findings.

  • Adds the missing Migrate permission to the default Admin permission set, so Admin capabilities can authorize package migration.
  • Adds regression coverage proving Admin permissions include migration.
  • Changes CountBased(N) delete-record locking to protect the last N records currently present in trail order, instead of using monotonic sequence-number distance.
  • Uses linked_table::back() and linked_table::prev() to evaluate the protected tail window after deletions.
  • Adds a tail-deletion regression test covering the case where a deleted tail record moves older records into the protected count window.
  • Updates Rust, WASM, and example docs to describe the current-record semantics and the linear gas trade-off for large count windows.

Links to any relevant issues

Type of change

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that new and existing unit tests pass locally with my changes
  • I have updated the CHANGELOG.md, if my changes are significant enough

@itsyaasir itsyaasir self-assigned this May 18, 2026
@itsyaasir itsyaasir requested a review from chrisgitiota May 18, 2026 08:07
@chrisgitiota
Copy link
Copy Markdown
Contributor

Regarding:

Adds a tail-deletion regression test covering the case where a deleted tail record moves older records into the protected count window.

In fun delete_records_batch() do we want to protect those moved older records even when they move into the window during the batch deletion?

itsyaasir and others added 3 commits May 19, 2026 12:57
)

- Rewrite of the `delete_records_batch()` and `is_record_locked()` functions to only evaluate against the existing records once when the transaction begins (has been evaluated for each deleted record before)
- `window_count_based(0)` is now forbidden in favor of using `window_none()` instead to prevent silently misconfigured trails.
-  Client-relevant asserts in Move `locking` module are validated in the AT Rust libraries public surface now

The Move locking module has two client-relevant asserts, now being also validated in the Rust crate:
  1. ECountWindowMustBePositive — window_count_based(0) is rejected.
  2. EUntilDestroyedNotSupportedForDeleteTrail — TimeLock::UntilDestroyed cannot be used as the trail-level delete lock (but it is valid for write_lock).

The tf_components::timelock module also asserts EPastTimestamp on unlock_at/unlock_at_ms when the timestamp is in the past. This is not mirrored in the Rust crate because it depends on the on-chain clock at execution time, so a client-side check would either be redundant (if the user picks a far-future timestamp) or wrong (if the transaction is built well before submission and a borderline timestamp lapses).

---------

Co-authored-by: Yasir <yasir@shariff.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants