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

# `proposeTransaction` is restricted to `onlyOwner`, blocking signers from proposing transactions

Author Revealed upon completion

proposeTransaction is restricted to onlyOwner, blocking signers from proposing transactions

Description

  • According to the README.md, any address holding the SIGNING_ROLE can propose a transaction.
    The permission is directly tied to the role, with no additional restrictions.

    Signers (holders of SIGNING_ROLE)
    Up to 5 addresses in total (owner + up to 4 others) that possess the SIGNING_ROLE.
    Powers:
    Confirm pending transaction proposals
    Revoke their own previous confirmation (useful if they change their mind before execution)
    Execute a transaction once:
    At least 3 distinct signers have confirmed, and
    The value-based timelock period has fully elapsed
    @> Propose new transactions (permission is tied to the role, so any signer can propose)

  • Despite the README.md stating that signers can propose transactions, the function is currently restricted to the owner (onlyOwner)

function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
@> onlyOwner //NONE SIGNERS CAN PROPOSE TRANSACTIONS, ONLY THE OWNER
returns (uint256)
{
return _proposeTransaction(to, value, data);
}

Risk

Likelihood:

  • Any signer attempting to propose a transaction will consistently be unable to do so due to the onlyOwner restriction.

Impact:

  • Signers cannot perform their intended role of proposing new transactions, limiting the operational functionality of the multisig.

  • The restriction increases centralization risk and may cause delays in transaction execution, as all proposals require owner intervention.

Proof of Concept

To demonstrate that the SIGNING_ROLE is not authorized to propose transactions, we invoke proposeTransaction using an account that holds this role (SIGNER_TWO) and expect the call to revert.

function testProposeWithSignerRole() public {
multiSigTimelock.grantSigningRole(SIGNER_TWO); //grant SIGNING_ROLE to SIGNER_TWO
address to = SPENDER_ONE;
uint256 value = 1 ether;
bytes memory data = "";
vm.startPrank(SIGNER_TWO);
​
vm.expectRevert();
multiSigTimelock.proposeTransaction(to, value, data); //propose with SIGNER_TWO
}

Then run :

forge test --mt testProposeWithSignerRole -vvvvv

The following output should be obtained :

Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest
[PASS] testProposeWithSignerRole() (gas: 99796)
Traces:
[2405280] MultiSigTimeLockTest::setUp()
├─ [2364461] → new MultiSigTimelock@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ ├─ emit RoleGranted(role: 0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, account: MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], sender: MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return] 11123 bytes of code
└─ ← [Stop]
​
[102596] MultiSigTimeLockTest::testProposeWithSignerRole()
├─ [83181] MultiSigTimelock::grantSigningRole(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ ├─ emit RoleGranted(role: 0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, account: signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa], sender: MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ ├─ storage changes:
│ │ @ 0x7ed2ae3fcc403bba9120be8a557eac30c02697b9ffb5faa455162a427324855b: 0 → 1
│ │ @ 2: 1 → 2
│ │ @ 4: 0 → 0x000000000000000000000000884d93ff97a786c23d91993e20382fc4c26bc2aa
│ │ @ 0x1b38d2010ad28ced672ce9dd1d9d2aba655d886d685d6ee276a49d66aedfac52: 0 → 1
│ └─ ← [Stop]
├─ [0] VM::startPrank(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error 0xf4844814)
│ └─ ← [Return]
├─ [4789] MultiSigTimelock::proposeTransaction(spender_one: [0x488dE584674d70E8Da70C42Cd4CC73Cd63d3dB23], 1000000000000000000 [1e18], 0x)
│ ├─ storage changes:
│ │ @ 0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00: 1 → 2
│ └─ ← [Revert] OwnableUnauthorizedAccount(0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa)
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.24ms (850.08µs CPU time)
​
Ran 1 test suite in 32.66ms (4.24ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

As expected, the call reverts with OwnableUnauthorizedAccount because the function is restricted to the owner.

Recommended Mitigation

Replace onlyOwner with onlyRole(SIGNING_ROLE). The owner also holds this role (as set in the constructor and stated in the README.md), so both the owner and other signers will be able to call this function, consistent with the intended behavior described in the README.md:

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!