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

`renounceRole()` bypasses signer tracking, causing state inconsistency

Author Revealed upon completion

renounceRole() bypasses signer tracking, causing state inconsistency

Description

The contract maintains internal signer tracking via s_signerCount, s_signers[], and s_isSigner[]. The custom revokeSigningRole() function properly updates these state variables.

However, the inherited renounceRole() from OpenZeppelin's AccessControl allows signers to renounce their role directly, bypassing the internal tracking update. This causes the AccessControl state and internal tracking to become inconsistent.

// OpenZeppelin's renounceRole is inherited and callable:
@> function renounceRole(bytes32 role, address callerConfirmation) public virtual {
@> // Only updates AccessControl state, not custom tracking (s_signerCount, s_signers[], s_isSigner[])
}

Risk

Likelihood: Medium

  • This occurs when any signer calls renounceRole(SIGNING_ROLE, signer) directly

Impact: High

  • State inconsistency between AccessControl roles and internal signer tracking

  • getSignerCount() returns incorrect value

  • getSigners() returns addresses that are no longer actually signers

  • Could be exploited to exceed the 5-signer limit after re-granting

Proof of Concept

Explanation: A signer calls renounceRole() directly instead of going through proper channels. The role is revoked at the AccessControl level, but s_signerCount remains unchanged, creating state inconsistency.

function test_RenounceRoleBypassesTracking() public {
multiSigTimelock.grantSigningRole(SIGNER_TWO);
​
uint256 countBefore = multiSigTimelock.getSignerCount();
assertEq(countBefore, 2);
​
// SIGNER_TWO renounces via inherited function
bytes32 signingRole = multiSigTimelock.getSigningRole();
vm.prank(SIGNER_TWO);
multiSigTimelock.renounceRole(signingRole, SIGNER_TWO);
​
// Role is revoked at AccessControl level
assertFalse(multiSigTimelock.hasRole(signingRole, SIGNER_TWO));
​
// BUG: Signer count NOT decremented - still 2, should be 1
assertEq(multiSigTimelock.getSignerCount(), 2);
}

Expected: getSignerCount() returns 1 after a signer renounces.
Actual: getSignerCount() still returns 2, creating state inconsistency.

Recommended Mitigation

Explanation: Override renounceRole() to block direct renunciation of SIGNING_ROLE. Users must use the proper revokeSigningRole() function which correctly updates all tracking state. This maintains consistency between AccessControl and internal state.

+ function renounceRole(bytes32 role, address callerConfirmation) public virtual override {
+ if (role == SIGNING_ROLE) {
+ revert("Use revokeSigningRole instead");
+ }
+ super.renounceRole(role, callerConfirmation);
+ }

Support

FAQs

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

Give us feedback!