Skip to content

LivenessModule - Slightly simplified code #17277

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
JosepBove merged 3 commits into from
Aug 29, 2025
Merged

LivenessModule - Slightly simplified code #17277

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4cfadc
Slightly simplified code
Aug 29, 2025
3c019aa
Removed empty line
Aug 29, 2025
6cf394c
Oops
Aug 29, 2025
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
37 changes: 18 additions & 19 deletions packages/contracts-bedrock/src/safe/LivenessModule2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ import { ILivenessModule2 } from "interfaces/safe/ILivenessModule2.sol";
/// 1. The Safe must first enable this module using ModuleManager.enableModule()
/// 2. The Safe must then configure the module by calling enableModule() with parameters
contract LivenessModule2 is ILivenessModule2 {

/// @notice Reserved address used as the previous owner to the first owner in a Safe
address public constant SENTINEL_OWNER = address(0x1);
Copy link
Contributor

@graphite-app graphite-app bot Aug 29, 2025

Choose a reason for hiding this comment

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

The constant SENTINEL_OWNER violates naming conventions for immutable variables. According to the Naming Conventions section, immutable variables should be declared as internal with a hand-written getter function rather than public visibility which automatically generates a getter. Change SENTINEL_OWNER to internal and add a custom getter function.

Suggested change
address public constant SENTINEL_OWNER = address(0x1);
address internal constant SENTINEL_OWNER = address(0x1);
/**
* @notice Returns the sentinel owner address.
* @return The sentinel owner address.
*/
function sentinelOwner() external pure returns (address) {
return SENTINEL_OWNER;
}

Spotted by Diamond (based on custom rule: Custom rules)

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.


/// @notice Configuration for a Safe's liveness module
struct SafeConfig {
uint256 livenessResponsePeriod;
Expand Down Expand Up @@ -171,34 +175,29 @@ contract LivenessModule2 is ILivenessModule2 {
Safe safe = Safe(payable(_safe));

// Get current owners
address[] memory currentOwners = safe.getOwners();
address[] memory owners = safe.getOwners();

if (currentOwners.length > 0) {
// Remove all owners except the first one from the front
// Remove all owners after the first one
while (owners.length > 1) {
// removeOwner automatically updates the threshold, so we don't need to do it manually
address sentinel = address(0x1); // Sentinel value for the first owner

for (uint256 i = currentOwners.length - 1; i > 0; i--) {
address ownerToRemove = currentOwners[0]; // Always remove the first owner
safe.execTransactionFromModule({
to: _safe,
value: 0,
operation: Enum.Operation.Call,
data: abi.encodeCall(OwnerManager.removeOwner, (sentinel, ownerToRemove, 1))
});
// Get updated owners list after removal
currentOwners = safe.getOwners();
}

// Now swap the remaining single owner with the fallback owner
safe.execTransactionFromModule({
to: _safe,
value: 0,
operation: Enum.Operation.Call,
data: abi.encodeCall(OwnerManager.swapOwner, (sentinel, currentOwners[0], config.fallbackOwner))
data: abi.encodeCall(OwnerManager.removeOwner, (SENTINEL_OWNER, owners[0], 1))
});
// Get updated owners list after removal
owners = safe.getOwners();
Comment on lines +181 to +190
Copy link
Contributor

@graphite-app graphite-app bot Aug 29, 2025

Choose a reason for hiding this comment

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

The current implementation has a logical issue. When removing owners[0] in each iteration, the array indices shift after each removal, causing the loop to always remove the first owner in the updated array. This doesn't align with the comment "Remove all owners after the first one" - it would actually remove all owners if run to completion.

To correctly implement the intended behavior, consider either:

  1. Iterating from the end of the array to index 1:
for (uint256 i = owners.length - 1; i > 0; i--) {
    // Remove owner at index i
}
  1. Or keeping the first owner by index or address:
address firstOwner = owners[0];
while (owners.length > 1) {
    // Find and remove an owner that isn't the first owner
}

The current approach will continue removing owners until potentially none remain, which could cause issues when trying to swap the "remaining single owner" later.

Suggested change
while (owners.length > 1) {
// removeOwner automatically updates the threshold, so we don't need to do it manually
address sentinel = address(0x1); // Sentinel value for the first owner
for (uint256 i = currentOwners.length - 1; i > 0; i--) {
address ownerToRemove = currentOwners[0]; // Always remove the first owner
safe.execTransactionFromModule({
to: _safe,
value: 0,
operation: Enum.Operation.Call,
data: abi.encodeCall(OwnerManager.removeOwner, (sentinel, ownerToRemove, 1))
});
// Get updated owners list after removal
currentOwners = safe.getOwners();
}
// Now swap the remaining single owner with the fallback owner
safe.execTransactionFromModule({
to: _safe,
value: 0,
operation: Enum.Operation.Call,
data: abi.encodeCall(OwnerManager.swapOwner, (sentinel, currentOwners[0], config.fallbackOwner))
data: abi.encodeCall(OwnerManager.removeOwner, (SENTINEL_OWNER, owners[0], 1))
});
// Get updated owners list after removal
owners = safe.getOwners();
address firstOwner = owners[0];
while (owners.length > 1) {
// Find an owner that isn't the first owner to remove
address ownerToRemove = owners[0] == firstOwner ? owners[1] : owners[0];
// removeOwner automatically updates the threshold, so we don't need to do it manually
safe.execTransactionFromModule({
to: _safe,
value: 0,
operation: Enum.Operation.Call,
data: abi.encodeCall(OwnerManager.removeOwner, (SENTINEL_OWNER, ownerToRemove, 1))
});
// Get updated owners list after removal
owners = safe.getOwners();

Spotted by Diamond

Fix in Graphite

Is this helpful? React 👍 or 👎 to let us know.

}

// Now swap the remaining single owner with the fallback owner
safe.execTransactionFromModule({
to: _safe,
value: 0,
operation: Enum.Operation.Call,
data: abi.encodeCall(OwnerManager.swapOwner, (SENTINEL_OWNER, owners[0], config.fallbackOwner))
});

// Reset the challenge state to allow a new challenge
config.challengeStartTime = 0;

Expand Down