Contract owner can revoke their own signer role, potentially freezing the protocol
Description
The revokeSigningRole function allows the contract owner to remove any signer from the whitelist, provided that the total number of signers remains greater than one.
* @dev Function to revoke signing role of an account. This function uses the "swap and pop" pattern for efficient array removal when order doesn't matter(in this case).
* @notice Adding the address(0) in place of the removed signer instead of leaving it blank, is not functionally necessary, but it's a best practice for gas efficiency, debugging clarity, and preventing future bugs.
* @notice Thereafter, granting signing roles would be no problem, the zero address would simply be overwritten.
* @param _account The address to be revoked of the signing role
*/
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);
}
}
However, there is no explicit check preventing the owner from revoking their own signer role. If the owner removes themselves and ownership is not transferred to another address, the contract may end up in a state where no address has owner privileges.
As a result, all functions protected by the onlyOwner modifier become permanently inaccessible.
Impact
If the owner revokes their own signer role, the protocol can become irreversibly frozen. Administrative actions such as managing signers, executing privileged operations, or maintaining the multisig configuration may no longer be possible.
This represents a permanent loss of control over the contract and can render the multisig wallet unusable.
Risk
Impact: High
Likelihood: Medium.
Reason 1. The exploit does not require an external attacker and depends on owner behavior. However, such situations commonly occur due to misconfiguration, operational mistakes, or automated scripts. Because the consequences are severe and irreversible, the risk remains significant.
Proof of Concept
Just copy and paste into the test file of the protocol:
function test_ownerCanRevokeSelfAndFreezeProtocol() public {
assertTrue(multiSigTimelock.isSigner(OWNER), "Owner should initially be a signer");
uint256 initialSignerCount = multiSigTimelock.getSignerCount();
assertGt(initialSignerCount, 1, "There must be more than one signer");
multiSigTimelock.revokeSigningRole(OWNER);
assertFalse(
multiSigTimelock.isSigner(OWNER),
"Owner was able to revoke their own signer role"
);
vm.expectRevert();
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
}
Recommended Mitigation
Explicitly prevent the owner from revoking their own signer role. For example, add a check that reverts if _account == owner(), or require ownership to be transferred to another address before revoking the owner’s signer permissions.
/**
* @dev Function to revoke signing role of an account. This function uses the "swap and pop" pattern for efficient array removal when order doesn't matter(in this case).
* @notice Adding the address(0) in place of the removed signer instead of leaving it blank, is not functionally necessary, but it's a best practice for gas efficiency, debugging clarity, and preventing future bugs.
* @notice Thereafter, granting signing roles would be no problem, the zero address would simply be overwritten.
* @param _account The address to be revoked of the signing role
*/
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
+ if (_account == owner) {
+ revert MultiSigTimelock__OwnerCantBeDeleted();
+ }
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}
