Root + Impact

• Root: The contract does not override AccessControl::renounceRole, allowing signers to revoke their own SIGNING_ROLE without updating the multisig's internal signer tracking variables.

• Impact: Internal signer bookkeeping (s_signerCount, s_signers, s_isSigner) becomes inconsistent with actual role assignments, leading to incorrect view data and potential governance deadlock.

Description

• Normal Behaviour: Signer membership should be managed exclusively through the multisig's administrative flow (grantSigningRole/revokeSigningRole), ensuring that both role assignments and internal signer state remain consistent.

• Issue: Since MultiSigTimelock inherits from OpenZeppelin's AccessControl without overriding renounceRole, any signer can call:

  // AccessControl, 155

  function renounceRole(bytes32 role, address callerConfirmation) public virtual {

  if (callerConfirmation != _msgSender()) {

  revert AccessControlBadConfirmation();

  }

  ​

  _revokeRole(role, callerConfirmation);

  }
  
  

  This removes the signer's role at the AccessControl level, but does not update the multisig's internal storage variables:

  • s_signerCount

  • s_signers

  • s_isSigner

  As a result, the contract maintains two conflicting sources of truth regarding signer membership.

Risk

• Likelihood: Low-Medium

  • Any signer can unintentionally or intentionally renounce their role.

  • No safeguards or warnings exist to prevent this action.

• Impact: Medium

  • View functions such as getSignerCount() and getSigners() return incorrect data.

  • The contract may believe the maximum signer count has been reached, preventing the owner from adding new signers.

  • Multiple self-renunciations can lead to governance deadlock with fewer active signers than expected.

  • The owner may remain unaware unless monitoring AccessControl::RoleRevoked events closely.

Proof Of Concept

• Add the following test to test/unit/MultiSigTimelockTest.t.sol

  function test__SignerRenounceRoleDesyncsInternalState() public grantSigningRoles {

  // Initial signer count

  uint256 initialCount = multiSigTimelock.getSignerCount();

  assertEq(initialCount, 5);

  console2.log("Initial signer count", initialCount);

  ​

  // SIGNER_TWO renounces their role directly via `renounceRole`

  vm.prank(SIGNER_TWO);

  multiSigTimelock.renounceRole(

  keccak256("SIGNING_ROLE"),

  SIGNER_TWO

  );

  ​

  // SIGNER_TWO has no longer any SIGNING_ROLE according to `AccessControl`

  assertFalse(

  multiSigTimelock.hasRole(

  multiSigTimelock.getSigningRole(),

  SIGNER_TWO

  )

  );

  console2.log("hasRole(SIGNING_ROLE, SIGNER_TWO):", multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_TWO));

  ​

  // But, the internal state still considers SIGNER_TWO as a signer

  assertEq(multiSigTimelock.getSignerCount(), initialCount);

  console2.log("Signer count after SIGNER_TWO renounces their role:", multiSigTimelock.getSignerCount());

  ​

  address[5] memory signers = multiSigTimelock.getSigners();

  bool signerStillPresent;

  ​

  for (uint256 i = 0; i < signers.length; i++) {

  if (signers[i] == SIGNER_TWO) {

  signerStillPresent = true;

  break;

  }

  }

  ​

  assertTrue(signerStillPresent);

  console2.log("SIGNER_TWO is still a signer:", signerStillPresent);

  ​

  // And the most unfortunate part, the owner can't even add a new signer

  // unless he revokes that same signer first, which doesn't even emit that `RoleRevoked` event.

  ​

  address newSigner = makeAddr("new_signer");

  vm.prank(OWNER);

  vm.expectRevert();

  multiSigTimelock.grantSigningRole(newSigner);

  console2.log("Adding new signer completely fails!!");

  }
  
  

• Run the test using

  forge test --mt test__SignerRenounceRoleDesyncsInternalState -vv
  
  

• Logs:

  Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest

  [PASS] test__SignerRenounceRoleDesyncsInternalState() (gas: 337572)

  Logs:

  Initial signer count 5

  hasRole(SIGNING_ROLE, SIGNER_TWO): false

  Signer count after SIGNER_TWO renounces their role: 5

  SIGNER_TWO is still a signer: true

  Adding a new signer completely fails!!

  ​

  Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.65ms (773.21µs CPU time)

Recommended Mitigation

Here are the two approaches one can take:

1. Disable self-renunciation for signers

   Override renounceRole to prevent signer self-removal completely. But then no one will be able to renounce roles in case of any malicious hack of their accounts

   + function renounceRole(bytes32 role, address account) public override {

   + require(role != SIGNING_ROLE, "Cannot renounce signing role directly");

   + super.renounceRole(role, account);

   + }
   
   

2. Route through multisig revocation logic

   Force signer self-removal to go through an alternate version of revokeSigningRole, ensuring internal state updates correctly.