Commit ec882b18 authored by Maurelian's avatar Maurelian

refactor(ctb): Fix placement of ownersCount

Also adds comments to clarify the logic.
parent 81a8c104
...@@ -114,19 +114,37 @@ contract LivenessModule is ISemver { ...@@ -114,19 +114,37 @@ contract LivenessModule is ISemver {
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");
// We will remove at least one owner, so we'll initialize the ownersCount count to the current number of owners // 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
// keep a local count of the number of owners this way.
uint256 ownersCount = SAFE.getOwners().length; uint256 ownersCount = SAFE.getOwners().length;
for (uint256 i = 0; i < _previousOwners.length; i++) { for (uint256 i = 0; i < _previousOwners.length; i++) {
ownersCount--; // 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
// of liveness,
// 2. the owner has not signed a transaction during the liveness interval.
if (ownersCount >= MIN_OWNERS) { if (ownersCount >= MIN_OWNERS) {
require(canRemove(_ownersToRemove[i]), "LivenessModule: the owner to remove has signed recently"); require(canRemove(_ownersToRemove[i]), "LivenessModule: the owner to remove has signed recently");
} }
// Pre-emptively update our local count of the number of owners.
// This is safe because _removeOwner will bubble up any revert from the Safe if the owner cannot be removed.
ownersCount--;
// We now attempt remove the owner from the safe.
_removeOwner({ _removeOwner({
_prevOwner: _previousOwners[i], _prevOwner: _previousOwners[i],
_ownerToRemove: _ownersToRemove[i], _ownerToRemove: _ownersToRemove[i],
_newOwnersCount: ownersCount _newOwnersCount: ownersCount
}); });
// when all owners are removed and the sole owner is the fallback owner, the
// ownersCount variable will be incorrectly set to zero.
// This reflects the fact that all prior owners have been removed. The loop should naturally exit at this
// point, but for safety we detect this condition and force the loop to terminate.
if (ownersCount == 0) {
break;
}
} }
_verifyFinalState(); _verifyFinalState();
} }
...@@ -182,7 +200,10 @@ contract LivenessModule is ISemver { ...@@ -182,7 +200,10 @@ contract LivenessModule is ISemver {
function _verifyFinalState() internal view { function _verifyFinalState() internal view {
address[] memory owners = SAFE.getOwners(); address[] memory owners = SAFE.getOwners();
uint256 numOwners = owners.length; uint256 numOwners = owners.length;
// Ensure that the safe is not being left in an unsafe state with too few owners.
// Ensure that the safe is not being left in a safe state such that either:
// 1. there are at least the minimum number of owners, or
// 2. there is a single owner and that owner is the fallback owner.
if (numOwners == 1) { if (numOwners == 1) {
require(owners[0] == FALLBACK_OWNER, "LivenessModule: must transfer ownership to fallback owner"); require(owners[0] == FALLBACK_OWNER, "LivenessModule: must transfer ownership to fallback owner");
} else { } else {
......
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