Commit e30ccc04 authored by Matt Solomon's avatar Matt Solomon Committed by GitHub

fix(ctb): kontrol snapshots (#10557)

* fix(ctb): snapshots

* ci: fix snapshot check command to also consider kontrol

* ctb: Disable the LivenessModule after ownership is transferred to
fallback owner

ctb: Use deactivated flag

ctb: Update snapshots

* ctb: Update LM deactivation
Co-authored-by: default avatarMatt Solomon <matt@mattsolomon.dev>

---------
Co-authored-by: default avatarMaurelian <maurelian@protonmail.ch>
Co-authored-by: default avatarMaurelian <john@oplabs.co>
parent 14151dbc
...@@ -5,9 +5,9 @@ set -euo pipefail ...@@ -5,9 +5,9 @@ set -euo pipefail
# Generate the snapshots # Generate the snapshots
pnpm snapshots pnpm snapshots
# Check if the generated snapshots are different from the committed snapshots # Check if the generated `snapshots` or `test/kontrol` files are different from the committed versions
if git diff --exit-code snapshots > /dev/null; then if git diff --exit-code snapshots test/kontrol > /dev/null; then
[ -z "$(git ls-files --others --exclude-standard snapshots)" ] || exit 1 [ -z "$(git ls-files --others --exclude-standard snapshots test/kontrol)" ] || exit 1
else else
exit 1 exit 1
fi fi
...@@ -112,8 +112,8 @@ ...@@ -112,8 +112,8 @@
"sourceCodeHash": "0xea3872d8f196ae3c863363dfa4b57803cb2a24b0c100244d8f861891e901e03f" "sourceCodeHash": "0xea3872d8f196ae3c863363dfa4b57803cb2a24b0c100244d8f861891e901e03f"
}, },
"src/Safe/LivenessModule.sol": { "src/Safe/LivenessModule.sol": {
"initCodeHash": "0xdf37345a47e176266f0ece4c624c7e9c2bd81661675f53cf07706dc44dafad27", "initCodeHash": "0xde144889fe7d98dbf300a98f5331edd535086a4af8ae6d88ca190c7f4c754a2d",
"sourceCodeHash": "0xdf17b5a6c068b2cf8fd24383066f0cc8e4ab0002f2470476beae594ca86879f3" "sourceCodeHash": "0x3ff4a3f21202478935412d47fd5ef7f94a170402ddc50e5c062013ce5544c83f"
}, },
"src/cannon/MIPS.sol": { "src/cannon/MIPS.sol": {
"initCodeHash": "0xa5d36fc67170ad87322f358f612695f642757bbf5280800d5d878da21402579a", "initCodeHash": "0xa5d36fc67170ad87322f358f612695f642757bbf5280800d5d878da21402579a",
......
...@@ -125,6 +125,19 @@ ...@@ -125,6 +125,19 @@
"stateMutability": "view", "stateMutability": "view",
"type": "function" "type": "function"
}, },
{
"inputs": [],
"name": "ownershipTransferredToFallback",
"outputs": [
{
"internalType": "bool",
"name": "",
"type": "bool"
}
],
"stateMutability": "view",
"type": "function"
},
{ {
"inputs": [ "inputs": [
{ {
......
[] [
\ No newline at end of file {
"bytes": "1",
"label": "ownershipTransferredToFallback",
"offset": 0,
"slot": "0",
"type": "bool"
}
]
\ No newline at end of file
...@@ -23,6 +23,9 @@ contract LivenessModule is ISemver { ...@@ -23,6 +23,9 @@ contract LivenessModule is ISemver {
/// @notice Emitted when the fallback owner takes ownership /// @notice Emitted when the fallback owner takes ownership
event OwnershipTransferredToFallback(); event OwnershipTransferredToFallback();
/// @notice Flag to indicate if the module has been deactivated
bool public ownershipTransferredToFallback;
/// @notice The Safe contract instance /// @notice The Safe contract instance
Safe internal immutable SAFE; Safe internal immutable SAFE;
...@@ -51,7 +54,7 @@ contract LivenessModule is ISemver { ...@@ -51,7 +54,7 @@ contract LivenessModule is ISemver {
/// @notice Semantic version. /// @notice Semantic version.
/// @custom:semver 1.1.0 /// @custom:semver 1.1.0
string public constant version = "1.1.0"; string public constant version = "1.2.0";
// Constructor to initialize the Safe and baseModule instances // Constructor to initialize the Safe and baseModule instances
constructor( constructor(
...@@ -131,11 +134,16 @@ contract LivenessModule is ISemver { ...@@ -131,11 +134,16 @@ contract LivenessModule is ISemver {
/// @param _ownersToRemove The owners to remove /// @param _ownersToRemove The owners to remove
function removeOwners(address[] memory _previousOwners, address[] memory _ownersToRemove) external { function removeOwners(address[] memory _previousOwners, address[] memory _ownersToRemove) external {
require(_previousOwners.length == _ownersToRemove.length, "LivenessModule: arrays must be the same length"); require(_previousOwners.length == _ownersToRemove.length, "LivenessModule: arrays must be the same length");
address[] memory currentOwners = SAFE.getOwners();
require(
!ownershipTransferredToFallback,
"LivenessModule: The safe has been shutdown, the LivenessModule and LivenessGuard should be removed or replaced."
);
// Initialize the ownersCount count to the current number of owners, so that we can track the number of // Initialize the ownersCount count to the current number of owners, so that we can track the number of
// owners in the Safe after each removal. The Safe will revert if an owner cannot be removed, so it is safe // owners in the Safe after each removal. The Safe will revert if an owner cannot be removed, so it is safe
// keep a local count of the number of owners this way. // keep a local count of the number of owners this way.
uint256 ownersCount = SAFE.getOwners().length; uint256 ownersCount = currentOwners.length;
for (uint256 i = 0; i < _previousOwners.length; i++) { for (uint256 i = 0; i < _previousOwners.length; i++) {
// Validate that the owner can be removed, which means that either: // Validate that the owner can be removed, which means that either:
// 1. the ownersCount is now less than MIN_OWNERS, in which case all owners should be removed regardless // 1. the ownersCount is now less than MIN_OWNERS, in which case all owners should be removed regardless
...@@ -196,6 +204,9 @@ contract LivenessModule is ISemver { ...@@ -196,6 +204,9 @@ contract LivenessModule is ISemver {
if (!success) { if (!success) {
revert OwnerRemovalFailed(string(returnData)); revert OwnerRemovalFailed(string(returnData));
} }
// Deactivate the module to prevent unintended behavior after the fallback owner has taken ownership.
ownershipTransferredToFallback = true;
emit OwnershipTransferredToFallback(); emit OwnershipTransferredToFallback();
} }
......
...@@ -115,6 +115,7 @@ contract LivenessModule_Getters_Test is LivenessModule_TestInit { ...@@ -115,6 +115,7 @@ contract LivenessModule_Getters_Test is LivenessModule_TestInit {
assertEq(livenessModule.thresholdPercentage(), THRESHOLD_PERCENTAGE); assertEq(livenessModule.thresholdPercentage(), THRESHOLD_PERCENTAGE);
assertEq(safeInstance.safe.getThreshold(), livenessModule.getRequiredThreshold(safeInstance.owners.length)); assertEq(safeInstance.safe.getThreshold(), livenessModule.getRequiredThreshold(safeInstance.owners.length));
assertEq(livenessModule.fallbackOwner(), fallbackOwner); assertEq(livenessModule.fallbackOwner(), fallbackOwner);
assertFalse(livenessModule.ownershipTransferredToFallback());
} }
} }
...@@ -400,6 +401,13 @@ contract LivenessModule_RemoveOwners_Test is LivenessModule_TestInit { ...@@ -400,6 +401,13 @@ contract LivenessModule_RemoveOwners_Test is LivenessModule_TestInit {
assertEq(safeInstance.safe.getOwners().length, 1); assertEq(safeInstance.safe.getOwners().length, 1);
assertEq(safeInstance.safe.getOwners()[0], fallbackOwner); assertEq(safeInstance.safe.getOwners()[0], fallbackOwner);
assertEq(safeInstance.safe.getThreshold(), 1); assertEq(safeInstance.safe.getThreshold(), 1);
// Ensure that the LivenessModule's removeOwners function is now disabled
assertTrue(livenessModule.ownershipTransferredToFallback());
vm.expectRevert(
"LivenessModule: The safe has been shutdown, the LivenessModule and LivenessGuard should be removed or replaced."
);
livenessModule.removeOwners(prevOwners, ownersToRemove);
} }
} }
...@@ -546,6 +554,12 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit { ...@@ -546,6 +554,12 @@ contract LivenessModule_RemoveOwnersFuzz_Test is LivenessModule_TestInit {
assertEq(safeInstance.safe.getOwners().length, 1); assertEq(safeInstance.safe.getOwners().length, 1);
assertEq(safeInstance.safe.getOwners()[0], fallbackOwner); assertEq(safeInstance.safe.getOwners()[0], fallbackOwner);
assertEq(safeInstance.safe.getThreshold(), 1); assertEq(safeInstance.safe.getThreshold(), 1);
// Ensure that the LivenessModule's removeOwners function is now disabled
assertTrue(livenessModule.ownershipTransferredToFallback());
vm.expectRevert(
"LivenessModule: The safe has been shutdown, the LivenessModule and LivenessGuard should be removed or replaced."
);
livenessModule.removeOwners(prevOwners, ownersToRemove);
} else { } else {
// For both of the incorrect behaviors, we need to calculate the number of owners to remove to // For both of the incorrect behaviors, we need to calculate the number of owners to remove to
// trigger that behavior. We initialize that value here then set it in the if statements below. // trigger that behavior. We initialize that value here then set it in the if statements below.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment