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

MultiSigTimelock Does Not Allow Non-owner Signers to Propose Transactions, In Violation of the Documentation

Author Revealed upon completion

MultiSigTimelock Does Not Allow Non-owner Signers to Propose Transactions, In Violation of the Documentation

Description

According to the README:

Signers (holders of SIGNING_ROLE)

...

  • Propose new transactions (permission is tied to the role, so any signer can propose)

But the code itself only allows the owner to propose new 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)

Risk

Likelihood:

  • High likelihood that a user who reads the documentation which allows signers to propose transactions would expect that signers are able to propose transactions

Impact:

  • This means that a malicious (or unavailable) owner can cause the wallet to be unable to make transactions.

Proof of Concept

The following unit test is from the included test suite.

function testProposeTransactionRevertsIfNonOwner() public {
// ARRANGE
address nonOwner = makeAddr("non_owner");
​
// ACT & ASSERT
vm.prank(nonOwner);
vm.expectRevert();
multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
}

Recommended Mitigation

Modify the MultiSigTimelock::proposeTransactionfunction to have an onlyRole(SIGNING_ROLE)modifier instead of the onlyOwnermodifier.

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!