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

Dual Ownership System Desynchronization

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: Ownable owner manages proposals/roles via onlyOwner. AccessControl SIGNING_ROLE holders confirm/execute via onlyRole(SIGNING_ROLE). Deployer starts with both.​

  • The issue: transferOwnership(newOwner) changes only Ownable.owner() - new owner lacks SIGNING_ROLE (cannot execute) and DEFAULT_ADMIN_ROLE (cannot manage roles), while old deployer retains full AccessControl powers.

// MultiSigTimelock.sol @> DUAL INHERITANCE - two independent permission systems @>
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol";
​
contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
// @> Constructor sets BOTH systems independently @>
constructor() Ownable(msg.sender) {
_grantRole(SIGNING_ROLE, msg.sender); // @> AccessControl SIGNING_ROLE @>
}
// @> onlyOwner() uses Ownable owner @>
function proposeTransaction(...) external onlyOwner { ... }
// @> onlyRole(SIGNING_ROLE) uses AccessControl @>
modifier onlyRole(bytes32 role) { _checkRole(role); _; }
}
​

Risk

Likelihood:

  • Occurs when owner calls transferOwnership() expecting full control transfer

  • Standard ownership transfer workflow triggers desynchronization automatically

Impact:

  • New owner cannot execute transactions despite onlyOwner proposal powers

  • Original deployer retains parallel admin control → centralization risk

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol";
import {MultiSigTimelock} from "src/MultiSigTimelock.sol";
​
contract DualOwnershipDesyncTest is Test {
MultiSigTimelock multiSigTimelock;
address deployer = makeAddr("deployer");
address newOwner = makeAddr("newOwner");
address signerA = makeAddr("signerA");
address signerB = makeAddr("signerB");
​
function setUp() public {
vm.prank(deployer);
multiSigTimelock = new MultiSigTimelock();
// Setup initial signers
vm.prank(deployer);
multiSigTimelock.grantSigningRole(signerA);
vm.prank(deployer);
multiSigTimelock.grantSigningRole(signerB);
console2.log("INITIAL STATE:");
console2.log(" Ownable owner:", multiSigTimelock.owner());
console2.log(" DEFAULT_ADMIN_ROLE:", multiSigTimelock.hasRole(multiSigTimelock.DEFAULT_ADMIN_ROLE(), deployer));
console2.log(" SIGNING_ROLE:", multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), deployer));
}
​
function testDualOwnershipDesynchronization() public {
bytes32 signingRole = multiSigTimelock.getSigningRole();
bytes32 adminRole = multiSigTimelock.DEFAULT_ADMIN_ROLE();
// ================= STEP 1: Deployer transfers Ownable ownership =================
vm.prank(deployer);
multiSigTimelock.transferOwnership(newOwner);
console2.log("AFTER transferOwnership(newOwner):");
​
// ================= PROOF: SPLIT-BRAIN PERMISSIONS =================
// Ownable owner changed [PASS]
assertEq(multiSigTimelock.owner(), newOwner, "Ownable.owner() -> newOwner");
// AccessControl roles UNCHANGED [FAIL]
assertFalse(multiSigTimelock.hasRole(adminRole, deployer), "Deployer: NO DEFAULT_ADMIN_ROLE (never had it)");
assertFalse(multiSigTimelock.hasRole(adminRole, newOwner), "NewOwner: NO DEFAULT_ADMIN_ROLE");
assertTrue(multiSigTimelock.hasRole(signingRole, deployer), "Deployer: STILL has SIGNING_ROLE");
assertFalse(multiSigTimelock.hasRole(signingRole, newOwner), "NewOwner: NO SIGNING_ROLE");
console2.log(" Ownable owner: newOwner [PASS]");
console2.log(" Deployer DEFAULT_ADMIN_ROLE: false [NOTE]");
console2.log(" NewOwner DEFAULT_ADMIN_ROLE: false [NOTE]");
console2.log(" Deployer SIGNING_ROLE: true [FAIL]");
console2.log(" NewOwner SIGNING_ROLE: false [NOTE]");
​
// ================= STEP 2: NewOwner can PROPOSE (onlyOwner) but cannot EXECUTE =================
vm.deal(address(multiSigTimelock), 10 ether);
// NewOwner CAN propose (Ownable owner)
vm.prank(newOwner);
uint256 txnId = multiSigTimelock.proposeTransaction(signerA, 5 ether, "");
console2.log("NewOwner CAN proposeTransaction [PASS] txnId:", txnId);
// NewOwner CANNOT confirm (no SIGNING_ROLE)
vm.prank(newOwner);
vm.expectRevert(); // MultiSigTimelockNotASigner
multiSigTimelock.confirmTransaction(txnId);
console2.log("NewOwner CANNOT confirmTransaction [FAIL] (no SIGNING_ROLE)");
// Deployer and signers STILL CAN confirm (retained SIGNING_ROLEs)
vm.prank(deployer);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(signerA);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(signerB);
multiSigTimelock.confirmTransaction(txnId);
vm.warp(block.timestamp + 2 days);
vm.prank(deployer);
multiSigTimelock.executeTransaction(txnId);
console2.log("Deployer STILL executes despite no longer Ownable owner [FAIL]");
​
// ================= STEP 3: NewOwner can grant/revoke roles (onlyOwner) =================
vm.prank(newOwner);
multiSigTimelock.grantSigningRole(newOwner); // Self-grant needed
console2.log("NewOwner grants self SIGNING_ROLE (centralized workaround) [PASS]");
​
// ================= PROOF: Deployer retains signing control despite ownership transfer =================
assertTrue(multiSigTimelock.hasRole(signingRole, deployer), "Deployer retains SIGNING_ROLE control");
console2.log("CRITICAL: Deployer retains SIGNING_ROLE -> can still execute transactions despite ownership transfer");
}
}
​

RESULT:

forge test --match-test testDualOwnershipDesynchronization -vvv
[⠢] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/testDualOwnershipDesynchronization.sol:DualOwnershipDesyncTest
[PASS] testDualOwnershipDesynchronization() (gas: 414607)
Logs:
INITIAL STATE:
Ownable owner: 0xaE0bDc4eEAC5E950B67C6819B118761CaAF61946
DEFAULT_ADMIN_ROLE: false
SIGNING_ROLE: true
AFTER transferOwnership(newOwner):
Ownable owner: newOwner [PASS]
Deployer DEFAULT_ADMIN_ROLE: false [NOTE]
NewOwner DEFAULT_ADMIN_ROLE: false [NOTE]
Deployer SIGNING_ROLE: true [FAIL]
NewOwner SIGNING_ROLE: false [NOTE]
NewOwner CAN proposeTransaction [PASS] txnId: 0
NewOwner CANNOT confirmTransaction [FAIL] (no SIGNING_ROLE)
Deployer STILL executes despite no longer Ownable owner [FAIL]
NewOwner grants self SIGNING_ROLE (centralized workaround) [PASS]
CRITICAL: Deployer retains SIGNING_ROLE -> can still execute transactions despite ownership transfer
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 835.03µs (228.91µs CPU time)
​
Ran 1 test suite in 8.63ms (835.03µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

Single AccessControl system eliminates desynchronization. Multisig governance via ADMIN_ROLE required for ownership changes.

// - remove this code
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
// Remove: is Ownable(msg.sender)
​
// + add this code - Pure AccessControl
bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
​
constructor() {
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(ADMIN_ROLE, msg.sender);
_grantRole(SIGNING_ROLE, msg.sender);
_setRoleAdmin(SIGNING_ROLE, ADMIN_ROLE); // Admin manages signers
}
​
function grantSigningRole(address account) external onlyRole(ADMIN_ROLE) {
// ... existing logic
}
​
// Replace onlyOwner → onlyRole(ADMIN_ROLE)
function proposeTransaction(...) external onlyRole(ADMIN_ROLE) { ... }
​

Support

FAQs

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

Give us feedback!