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

Inherited AccessControl Functions Create Security Confusion

Author Revealed upon completion

Description

  • The contract inherits OpenZeppelin's AccessControl but implements custom role management functions (grantSigningRole, revokeSigningRole).

  • The inherited grantRole() and revokeRole() functions remain public and callable, but they don't update the custom s_signers array and s_isSigner mapping.

  • This creates an inconsistent state where AccessControl and custom tracking can become desynchronized.

contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
// Custom tracking
address[5] private s_signers;
mapping(address user => bool signer) private s_isSigner;
// @> Inherited from AccessControl - still public!
// function grantRole(bytes32 role, address account) public virtual
// function revokeRole(bytes32 role, address account) public virtual
// Custom functions that update both AccessControl AND custom tracking
function grantSigningRole(address _account) external { ... }
function revokeSigningRole(address _account) external { ... }
}

Risk

Likelihood:

  • Currently mitigated because no one has DEFAULT_ADMIN_ROLE

  • However, if Missing DEFAULT_ADMIN_ROLE Grant in Constructor is fixed (granting DEFAULT_ADMIN_ROLE), this becomes exploitable

  • Integration confusion - other contracts might call inherited functions

Impact:

  • State desynchronization between AccessControl roles and custom signer tracking

  • Someone with SIGNING_ROLE via inherited grantRole() won't be in s_signers array

  • Could bypass max signer limit (5) via inherited functions

  • getSigners() returns incorrect data

  • Execution validation could fail unexpectedly

Proof of Concept

function testAccessControlDesync() public {
// Assume DEFAULT_ADMIN_ROLE was granted (H-03 fixed)
// For this PoC, we'll use internal call simulation
// Setup: 5 signers already exist (max reached)
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
multiSigTimelock.grantSigningRole(SIGNER_FOUR);
multiSigTimelock.grantSigningRole(SIGNER_FIVE);
assertEq(multiSigTimelock.getSignerCount(), 5); // Max reached
// If admin could call inherited grantRole directly:
// bytes32 signingRole = multiSigTimelock.getSigningRole();
// multiSigTimelock.grantRole(signingRole, address(0x6thSigner));
// This would:
// 1. Grant SIGNING_ROLE to 6th signer via AccessControl
// 2. NOT update s_signers array (still shows 5)
// 3. NOT update s_isSigner mapping
// 4. NOT increment s_signerCount
// Result: 6th signer has role but isn't tracked
// They can confirm/execute but aren't in getSigners()
}

Recommended Mitigation

Override inherited AccessControl functions to prevent direct usage:

+ // Disable inherited AccessControl public functions
+ function grantRole(bytes32, address) public pure override {
+ revert("Use grantSigningRole");
+ }
​
+ function revokeRole(bytes32, address) public pure override {
+ revert("Use revokeSigningRole");
+ }
​
+ function renounceRole(bytes32, address) public pure override {
+ revert("Cannot renounce");
+ }

Or remove AccessControl inheritance entirely and use a simpler role check:

- contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
+ contract MultiSigTimelock is Ownable, ReentrancyGuard {
​
+ modifier onlySigner() {
+ require(s_isSigner[msg.sender], "Not a signer");
+ _;
+ }

Support

FAQs

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

Give us feedback!