MultiSig Timelock

First Flight #55
Beginner FriendlyWallet
100 EXP
Submission Details
Impact: high
Likelihood: high

Signers can't propose transactions

Author Revealed upon completion

Incorrect usage of onlyOwner modifier in MultiSigTimeLock::proposeTransaction() disallows signers to propose transactions

Description

  • Normal behavior - Signers which are granted the role SIGNING_ROLE must be able to propose transactions , it is also stated in the docs -> "Propose new transactions (permission is tied to the role, so any signer can propose)"

  • Issue - Transaction proposal restricted to Owner only. The proposeTransaction function is restricted to the contract owner, preventing signers from initiating transactions. As a result, even though multiple signers exist to confirm and execute transactions, only the owner is able to create proposals.

    This design significantly limits the effectiveness of the multisignature mechanism and introduces a centralization point where the owner becomes a mandatory intermediary for all transaction initiation.

// @audit only owner can propose transactions, signers can't
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
@> onlyOwner
returns (uint256)
{
return _proposeTransaction(to, value, data);
}

Risk

Likelihood:

  • Likelihood - High: This behavior occurs as part of the contract’s normal execution flow. Any attempt to propose a transaction from an address holding the SIGNING_ROLE is going to revert.


Impact:

  • Impact - High. Signers are expected to be able to propose transactions. However, due to the current access control limitation, only the owner is permitted to initiate proposals. This prevents authorized signers from performing a core function of the system, effectively centralizing control and disrupting the intended multisignature mechanism

Proof of Concept

  1. Owner grants signing role to an address

  2. The signer which was granted permission should now be able to propose a transaction

  3. The signers uses MultiSigTimeLock.sol::proposeTransaction() but it is going to revert.

    function test_failToProposeTx() public {
    multiSigTimelock.grantSigningRole(SIGNER_TWO);
    // signers count should be 2
    assertEq(multiSigTimelock.getSignerCount(), 2);
    address receiver = 0x93923B42Ff4bDF533634Ea71bF626c90286D27A0;
    uint256 amount = 5e15;
    bytes memory DATA = "";
    vm.startPrank(SIGNER_TWO);
    vm.expectRevert();
    multiSigTimelock.proposeTransaction(receiver, amount, DATA);
    }

Recommended Mitigation

Do not use onlyOwner but instead check for the role in the 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!