Skip to content

Conversation

@maurelian
Copy link
Contributor

@maurelian maurelian commented Oct 9, 2025

Summary

This PR adds the following functionality:

  • disable the guard when the fallback owner takes over

In this process, I also realized that we should expose a clearTimelockGuard() function which parallels clearLivenessModule() to:

  • deconfigure the TimelockGuard
  • cancel all pending transactions in the timelock guard and delete the timelock config

@maurelian maurelian requested a review from a team as a code owner October 9, 2025 19:23
@maurelian maurelian requested a review from ajsutton October 9, 2025 19:23
@maurelian maurelian force-pushed the jm/disable-extensions-on-transfer branch from cf4a600 to 3ffede7 Compare October 9, 2025 19:52
@maurelian maurelian requested review from a team as code owners October 9, 2025 19:52
@maurelian maurelian changed the base branch from develop to jm/combined-livenessmodule2-timelockguard October 9, 2025 19:53
@maurelian maurelian force-pushed the jm/combined-livenessmodule2-timelockguard branch from 2ff3b8c to 9efa607 Compare October 9, 2025 20:05
@maurelian maurelian force-pushed the jm/disable-extensions-on-transfer branch from 3ffede7 to 2f291e2 Compare October 9, 2025 20:10
Move semver to SaferSafes

semver lock
@maurelian maurelian force-pushed the jm/combined-livenessmodule2-timelockguard branch from 37e90d6 to 05aa13d Compare October 9, 2025 20:27
@maurelian maurelian force-pushed the jm/disable-extensions-on-transfer branch from 6001ae5 to 67bddc5 Compare October 9, 2025 20:29
It's no longer reused in the change ownership call.
Copy link
Contributor

@alcueca alcueca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me, I think this is the safest implementation.

@maurelian maurelian changed the title Disable the guard and module upon ownership transfer Disable the guard upon ownership transfer Oct 17, 2025
@maurelian maurelian added this pull request to the merge queue Oct 18, 2025
Merged via the queue into develop with commit 7221832 Oct 18, 2025
71 checks passed
@maurelian maurelian deleted the jm/disable-extensions-on-transfer branch October 18, 2025 00:39
ajsutton pushed a commit that referenced this pull request Oct 20, 2025
* Add SaferSafes as child of the module and guard

* Add ISaferSafes

* Test comment and assertion fixes

* Improve comments

* Make LivenessModule2 and TimelockGuard abstract

Move semver to SaferSafes

semver lock

* fix test contract name

* Move semver to SaferSafes

* Disable the guard and module upon ownership transfer

* Add _disableThisGuard function

* Update tests

* Add config resets

* fmt

* fix test_changeOwnershipToFallback_canRechallenge_succeeds

* Simplify by clearing config directly

* Put _disableThisGuard into child contract

* Add timelockDelay reset on _disableThisGuard

* semver-lock

* Move _disableThisGuard logic into TimelockGuard

* clear livenessSafeConfig at tend of _disableThisModule

* Clarify use of SENTINEL_OWNER

* Fix the ordering of the disableGuard and disableModule calls

* semver-lock

* remove unused imports

* rename _disableThisGuard to _disableGuard

* bump semver

* Add test to remove unrelated guard

* Add SENTINEL_MODULE constant

* Clean up using ternary if

* Reset cancellationThreshold to 0 on changeOwnership

* Fix moduleFound if/else handling

* Clear pending transactions

* Pre-pr fixes

* Add test contract to test name lint exclusions

* fix name of test contract

* Move _disableGuard impl into TimelockGuard

* Add missing natspec

* Add gas limit testing on changeOwnershipToFallback

* Remove interfaces for abstract contracts

* Move state changes out into internal _clearLivenessModule

* Improve names on the internal _disableX methods

* Add clearTimelockGuard function

* Add _disableGuard helper to TLG tests

* Limit number of transactions cancelled to 100

* Revert "Remove interfaces for abstract contracts"

This reverts commit bd03288.

* Move livenessModule2 address into TestUtils

Reduces diff a bit

* Reduce diff somewhat

* Remove unused arg

* Update packages/contracts-bedrock/src/safe/TimelockGuard.sol

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>

* Fix iface

* update abi for iface fix

* Do not clear or disable the module during ownership transfer

* Fix inaccurate comment on _disableAndClearGuard

* Further improve comment

* remove unused import

* fix test name

* Do not clear guard during changeOwnershipToFallback

* Remove unused SENTINEL_MODULE var

* Remove dangling comment

* Revert "Remove dangling comment"

This reverts commit d266d12.

* Fix whitespace

* remove unnecessary internal _clearTimelockGuard function

It's no longer reused in the change ownership call.

* Address feedback

* Add missing assertion

* Move guard slot into constants

* semver-lock

* Remove LivenessModule from semver-lock

* fix: fmt, semver-lock, unused imports

* Remove unused variable

* fix semver lock by resetting old LivenessModule

* fix unused import

---------

Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
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.

4 participants