MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: high

Protocol can revoke the owner signer role thus break one of the invariant

Author Revealed upon completion

Protocol can revoke the owner signer role thus break one of the invariant

Description

The revokeSigningRole function allows the protocol owner to revoke their own signer role, as long as at least one other signer remains. This breaks one of protocol invariant that the owner should always retain signing capabilities.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) { // <@ NOT CHECKING THE OWNER ADDRESS
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// ... (the rest of the codes)
}

Impact

Low – The owner can re-grant the signer role to themselves, but this creates a temporary loss of control and violates system design assumptions.

While the owner can restore their role, revoking it even temporarily:

  • Breaks the invariant that the owner always maintains signing authority

  • Removes the owner’s ability to confirm, revoke, or execute transactions until the role is restored

  • Introduces an unexpected state that could disrupt governance or emergency operations


Proof of Concepts

Add this function into the existing unit test

Run the function with forge test --match-test testRevokeOwnerAsSigner -vvvv

// As long as the total signer >= 2 still can revoke owner role as signer
function testRevokeOwnerAsSigner() public grantSigningRoles {
multiSigTimelock.revokeSigningRole(SIGNER_FOUR);
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
​
// revoke owner role
multiSigTimelock.revokeSigningRole(multiSigTimelock.owner());
console2.log("contract owner address ", multiSigTimelock.owner());
​
address[5] memory signers = multiSigTimelock.getSigners();
for (uint8 i = 0; i<5; i++){
console2.log("signer address is ", signers[i]);
}
assertEq(multiSigTimelock.getSignerCount(), 1);
}

Recommended mitigation

Change the logic function to check the owner address instead

// Consider renaming the error to MultiSigTimelock__CannotRevokeOwner() for clarity
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
- if (_s_signerCount <= 1_) {
+ if (_account == owner()) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// ... (the rest of the codes)
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!