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

Owner-only proposal violates multi-sig specification leading to centralization risk

Author Revealed upon completion

Owner-only proposal violates multi-sig specification leading to centralization risk

Description

  • The specification states: Propose new transactions (permission is tied to the role, so any signer can propose).

  • However, the MultiSigTimelock::proposeTransaction function has an onlyOwner modifier, making it owner-exclusive.

  • The owner can unilaterally propose any transaction, and then simply wait for 2 other signers (totalling 3) to confirm it, effectively controlling all outgoing transactions

// Root cause in the codebase with @> marks to highlight the relevant section
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
@> onlyOwner
returns (uint256)
{
return _proposeTransaction(to, value, data);
}

Risk

Likelihood:

  • High. Only onwer can propose transactions, since this is what the code states.

Impact:

  • Critical. This transforms the contract from a true multi-signature wallet to an owner-controlled wallet with advisory confirmations.

  • The owner becomes a single point of failure/control, defeating the core purpose of multi-signature security.

Proof of Concept

As demonstrated in the test case below, only the owner can propose transactions. If a non-owner attempts to propose a transaction, it results in a revert, showcasing the violation of the multi-signature principle.

Paste the following test case into test/MultiSigTimelock.t.sol to reproduce:

function testOnlyOwnerCanProposeTransaction() public {
vm.deal(OWNER, OWNER_BALANCE_ONE);
​
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
assertEq(txnId, 0);
​
vm.prank(OWNER);
uint256 txnIdTwo = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
assertEq(txnIdTwo, 1);
​
address nonOwner = makeAddr("non_owner");
vm.prank(nonOwner);
//Expect revert since only owner can propose making multi-sig ineffective
vm.expectRevert();
multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
}

Recommended Mitigation

Replace onlyOwner with onlyRole(SIGNING_ROLE) in MultiSigTimelock::proposeTransaction

function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
- onlyOwner
+ onlyRole(SIGNING_ROLE)
returns (uint256)
{
return _proposeTransaction(to, value, data);
}

Support

FAQs

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

Give us feedback!