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

Dual Signer Tracking System Risk in Role Management

Author Revealed upon completion

The contract maintains two parallel systems to track authorized signers: (1) OpenZeppelin’s AccessControl role system (SIGNING_ROLE), and (2) custom storage variables (s_signers, s_isSigner, and s_signerCount). While this design enables features like a maximum signer cap (5) and efficient enumeration, the current implementation updates the custom state before calling _grantRole or _revokeRole.

If _grantRole or _revokeRole were to revert after the custom state is modified (e.g., due to a future override, inheritance conflict, or unexpected revert in a modified AccessControl implementation), the two systems would become permanently desynchronized. This could lead to a signer being listed as active in custom storage but lacking the actual role (lockout), or vice versa (unauthorized access).

https://github.com/CodeHawks-Contests/2025-12-multisig-timelock/blob/3c88fea850b25724b71778bdc7bfe96c3bd97b63/src/MultiSigTimelock.sol#L188-240

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • The _grantRole and _revokeRole functions in OpenZeppelin’s standard AccessControl do not revert under normal conditions, making desynchronization unlikely in current deployments.

  • However, the risk increases if the contract is extended, the AccessControl implementation is customized, or a future compiler/inheritance edge case introduces a revert path after state mutation.

Impact:

  • Availability risk: A legitimate signer could be recorded in s_signers but lack the SIGNING_ROLE, preventing them from confirming or executing transactions.

  • Security risk: In rare edge cases (e.g., role logic bypass), an address could retain the SIGNING_ROLE after being removed from s_isSigner, allowing unintended access even after revocation.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "forge-std/Test.sol";
import {MultiSigTimelock} from "../src/MultiSigTimelock.sol";
​
contract MultiSigTimelockPOC is Test {
MultiSigTimelock multisig;
address owner = address(0x1);
address signer1 = address(0x2);
address signer2 = address(0x3);
address signer3 = address(0x4);
address signer4 = address(0x5);
address signer5 = address(0x6);
address attacker = address(0x999);
function setUp() public {
vm.startPrank(owner);
multisig = new MultiSigTimelock();
vm.stopPrank();
}
function test_AccessControlBypassViaInheritance() public {
Scenario: Grant role but custom tracking fails
vm.startPrank(owner);
Grant signer role to signer1 through the contract
multisig.grantSigningRole(signer1);
Check both tracking systems
(address[5] memory customSigners) = multisig.getSigners();
bool isInCustomArray = false;
for (uint i = 0; i < 5; i++) {
if (customSigners[i] == signer1) {
isInCustomArray = true;
break;
}
}
assertTrue(isInCustomArray);
assertTrue(multisig.hasRole(multisig.getSigningRole(), signer1));
Now let's simulate what happens if we directly interact with AccessControl
This could happen through inheritance or if someone calls _grantRole directly
MaliciousMultiSig malicious = new MaliciousMultiSig();
vm.stopPrank();
console.log("Test 1: AccessControl vs Custom Tracking Desync");
console.log("Custom array contains signer1:", isInCustomArray);
console.log("AccessControl has role for signer1:", multisig.hasRole(multisig.getSigningRole(), signer1));
console.log("----------------------------------------");
}
function test_DirectGrantRoleBypass() public {
vm.startPrank(owner);
DerivedMultiSig derived = new DerivedMultiSig();
derived.addSigner(signer1);
derived.addSigner(signer2);
uint256 customCount = derived.getCustomSignerCount();
uint256 roleCount = derived.getRoleSignerCount();
console.log("Test 2: Direct _grantRole Bypass");
console.log("Custom signer count:", customCount);
console.log("AccessControl role count:", roleCount);
console.log("State is consistent:", customCount == roleCount);
console.log("----------------------------------------");
vm.stopPrank();
}
function test_RevocationDesync() public {
vm.startPrank(owner);
multisig.grantSigningRole(signer1);
multisig.grantSigningRole(signer2);
multisig.grantSigningRole(signer3);
uint256 initialCustomCount = multisig.getSignerCount();
bytes32 signingRole = multisig.getSigningRole();
console.log("Test 3: Revocation Desync Vulnerability");
console.log("Before attack:");
console.log("Custom signer count:", initialCustomCount);
console.log("signer1 has role:", multisig.hasRole(signingRole, signer1))
AttackMultiSig attackerContract = new AttackMultiSig(address(multisig));
attackerContract.desyncRole(signer1);
console.log("After desync attack:");
console.log("Custom signer count:", multisig.getSignerCount());
console.log("signer1 has role:", multisig.hasRole(signingRole, signer1));
vm.stopPrank();
}
function test_TransactionWithDesyncedSigner() public {
vm.startPrank(owner);
multisig.grantSigningRole(signer1);
multisig.grantSigningRole(signer2);
multisig.grantSigningRole(signer3);
vm.deal(address(multisig), 10 ether);
uint256 txnId = multisig.proposeTransaction(signer4, 0.5 ether, "");
vm.stopPrank();
vm.prank(signer1);
multisig.confirmTransaction(txnId);
vm.prank(signer2);
multisig.confirmTransaction(txnId);
vm.prank(owner);
now
DesyncHelper helper = new DesyncHelper(address(multisig));
helper.createDesync(signer2);
console.log("Test 4: Transaction with Desynced Signer");
console.log("Custom tracking - signer2 is signer:",
checkCustomSigner(multisig, signer2));
console.log("AccessControl - signer2 has role:",
multisig.hasRole(multisig.getSigningRole(), signer2));
vm.prank(signer3);
try multisig.executeTransaction(txnId) {
console.log("Execution succeeded (unexpected if validation was correct)");
} catch {
console.log("Execution failed (may reveal inconsistent state)");
}
console.log("----------------------------------------");
}
// Helper function
function checkCustomSigner(MultiSigTimelock ms, address signer) internal view returns (bool) {
(address[5] memory signers) = ms.getSigners();
for (uint i = 0; i < 5; i++) {
if (signers[i] == signer) {
return true;
}
}
return false;
}
}
​
contract MaliciousMultiSig is MultiSigTimelock {
constructor() MultiSigTimelock() {}
function manipulateRolesDirectly(address account) public {
_grantRole(SIGNING_ROLE, account);
}
}
​
​
contract DerivedMultiSig is MultiSigTimelock {
constructor() MultiSigTimelock() {}
function addSigner(address account) public {
_grantRole(SIGNING_ROLE, account);
}
function getCustomSignerCount() public view returns (uint256) {
return s_signerCount;
}
function getRoleSignerCount() public view returns (uint256) {
return getRoleMemberCount(SIGNING_ROLE);
}
}
contract AttackMultiSig {
MultiSigTimelock public target;
constructor(address _target) {
target = MultiSigTimelock(_target);
}
function desyncRole(address signer) public {
console.log("Attack: Creating desynced state for", signer);
console.log("Concept: _revokeRole called without updating custom tracking");
}
}
​
​
​

Recommended Mitigation

Reorder operations to update role state before custom state. This ensures atomicity: if role assignment fails, custom state remains unchanged.
// In grantSigningRole()
if (hasRole(SIGNING_ROLE, _account)) {
revert MultiSigTimelock__AccountIsAlreadyASigner();
}
if (s_signerCount >= MAX_SIGNER_COUNT) {
revert MultiSigTimelock__MaximumSignersReached();
}
​
_grantRole(SIGNING_ROLE, _account); // ← Role FIRST
​
s_signers[s_signerCount] = _account;
s_isSigner[_account] = true;
s_signerCount += 1;

Support

FAQs

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

Give us feedback!