Description
-
The contract intends the deployer/owner() to be the first signer and, per comments, should not be revocable as a signer to avoid breaking the multisig invariants and governance flow.
-
revokeSigningRole only prevents removing the last remaining signer (s_signerCount <= 1) but does not specially protect the owner. As long as there are ≥2 signers, the owner can be revoked from SIGNING_ROLE. This contradicts the intent expressed in comments and can degrade operations (the owner can no longer confirm/execute), and in small signer sets it can collapse quorum and lock funds.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}
Risk
Likelihood: Medium
-
In ordinary role maintenance (e.g., adding one more signer), calling revokeSigningRole(owner()) is possible and will succeed whenever there are at least two signers total.
-
This occurs during routine admin actions, incident response, or by mistake - because no explicit guard blocks targeting the owner.
Impact: High
-
Operational degradation: The owner loses the ability to confirmTransaction/executeTransaction (both gated by onlyRole(SIGNING_ROLE)), reducing available signers and potentially stalling governance.
-
Lock risk: In small signer sets, revoking the owner can reduce the number of usable signers below the fixed quorum (REQUIRED_CONFIRMATIONS = 3), making transactions unexecutable until additional signers are granted.
Proof of Concept
function testOwnerCanRevokeTheirOwnSigningRole() public {
bytes32 SIGNING_ROLE = multiSigTimelock.getSigningRole();
uint256 signerCount = multiSigTimelock.getSignerCount();
assertEq(signerCount, 1);
console2.log("Initial signer count after contract deployment:", signerCount);
multiSigTimelock.grantSigningRole(SIGNER_TWO);
signerCount = multiSigTimelock.getSignerCount();
assertEq(signerCount, 2);
console2.log("Signer count after granting SIGNER_TWO:", signerCount);
bool isSigner = multiSigTimelock.hasRole(SIGNING_ROLE, SIGNER_TWO);
assertTrue(isSigner);
console2.log("Is SIGNER_TWO a signer?", isSigner);
multiSigTimelock.revokeSigningRole(OWNER);
signerCount = multiSigTimelock.getSignerCount();
assertEq(signerCount, 1);
console2.log("Signer count after revoking OWNER:", signerCount);
isSigner = multiSigTimelock.hasRole(SIGNING_ROLE, OWNER);
assertFalse(isSigner);
console2.log("Is OWNER a signer?", isSigner);
}
Output:
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest
[PASS] testOwnerCanRevokeTheirOwnSigningRole() (gas: 108088)
Logs:
Initial signer count after contract deployment: 1
Signer count after granting SIGNER_TWO: 2
Is SIGNER_TWO a signer? true
Signer count after revoking OWNER: 1
Is OWNER a signer? false
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.16ms (3.06ms CPU time)
Ran 1 test suite in 102.16ms (16.16ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
+ // Disallow revoking the owner's signing role to preserve governance invariants
+ if (_account == owner()) {
+ revert MultiSigTimelock__CannotRevokeOwnerSigner();
+ }
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
// ... existing removal logic ...
}
