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

Centralization Risk: Excessive Owner Privileges Lead to Governance Bypass

mafa

Centralization Risk: Excessive Owner Privileges Lead to Governance Bypass

Description

The contract lacks sufficient restrictions on the Owner's privileges, leading to a critical centralization risk. The Owner maintains absolute authority to unilaterally change the contract's ownership and modify the signingRole by adding or revoking signers at will. This excessive power allows a malicious Owner to replace legitimate signers with controlled addresses, satisfying the multi-signature requirement to bypass security protocols.

Risk

Impact：

  1. Transfer Ownership: Change the contract's Owner at any time without consensus.

  2. Manipulate Signing Roles: Arbitrarily call grantSigningRole and revokeSigningRole. This allows a malicious owner to remove legitimate signers and replace them with controlled "sybil" accounts.

Proof of Concept

The Owner can unilaterally revoke two honest signers and grant signing roles to two controlled sub-wallets. After transferring contract ownership to one of these sub-wallets, the Owner would maintain signing authority across three separate accounts. They could then propose a transfer of all funds to their own address and provide all three required confirmations using these controlled wallets, bypassing the multisig consensus to drain the vault.

place the following code in MultiSigTimelockTest.t.sol:


function testCentralization() public grantSigningRoles {
​
vm.deal(address(multiSigTimelock), 100 ether);
​
// assume owner have another two wallet
address OWNER_SECOND_WALLET= makeAddr("owner_2");
address OWNER_THIRD_WALLET= makeAddr("owner_3");
​
vm.startPrank(OWNER);
// revoke the other signers
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
// grant his other wallet signing roll
multiSigTimelock.grantSigningRole(OWNER_SECOND_WALLET);
multiSigTimelock.grantSigningRole(OWNER_THIRD_WALLET);
//transfer ownership to another wallet from owner
multiSigTimelock.transferOwnership(OWNER_SECOND_WALLET);
vm.stopPrank();
​
// new owner of the contract
vm.startPrank(OWNER_SECOND_WALLET);
// malicious propose
uint256 trx_Id=multiSigTimelock.proposeTransaction(OWNER_THIRD_WALLET,100 ether,"");
multiSigTimelock.confirmTransaction(trx_Id);
vm.stopPrank();
// owner still have the SigningRole
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(trx_Id);
​
vm.warp(block.timestamp + 7 days);
​
vm.startPrank(OWNER_THIRD_WALLET);
multiSigTimelock.confirmTransaction(trx_Id);
multiSigTimelock.executeTransaction(trx_Id);
​
//drain the money
assertEq(address(multiSigTimelock).balance,0);
​
MultiSigTimelock.Transaction memory trx = multiSigTimelock.getTransaction(trx_Id);
assert(trx.confirmations >= 3);
assert(trx.executed);
​
}

Recommended Mitigation

Remove the onlyOwner modifier from sensitive administrative functions and replace it with a onlySelf modifier that restricts access to the contract itself. Functions such as grantSigningRole, revokeSigningRole, and transferOwnership should only be executable via the executeTransaction.

+ modifier onlySelf() {
+ require(msg.sender == address(this), "Must be executed via multisig process");
+ _;
+ }
​
Updates

Lead Judging Commences

kelechikizito Lead Judge 4 days ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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

Give us feedback!