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

Signers other than the owner cannot propose any new transaction

Author Revealed upon completion

Root + Impact

Description

  • Even though it is clearly said in the documentation that ALL signers have to be able to propose new transactions, due to applying the onlyOwner modifier to the MultiSigTimelock::proposeTransaction function, the signers other than the owner cannot do it.

  • Therefore, the other signers cannot use the power granted to them in the plan.

  • They have to wait for the owner to propose a new transaction. So, the protocol cannot continue its ususal business without the active participation of the owner.

@> function proposeTransaction(address to, uint256 value, bytes calldata data) external nonReentrant noneZeroAddress(to) onlyOwner returns (uint256) {
return _proposeTransaction(to, value, data);
}

Risk

Likelihood: High

  • Since it is about the incorrect implementation of the business logic and is not related to any specific condition, it certainly happens.

Impact:

  • Majority of the signers would not be able to propose any new transactions, so they would have to wait for the owner to do this task.

  • It breaks one of the invariants of the protocol which is about the power all signers should have.

Proof of Concept

Please copy the following function to the MultiSigTimelockTest.t.sol test file and run it with forge test --mt testSignersCanNotProposeTransaction -vvvv command in the terminal.

import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
.
.
.
function testSignersCanNotProposeTransaction() public {
// ARRANGE
vm.deal(SIGNER_TWO, OWNER_BALANCE_ONE);
​
// ACT & ASSERT
vm.prank(SIGNER_TWO);
vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, SIGNER_TWO));
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
}

Recommended Mitigation

To solve this issue you just need to replace the onlyOwner modifier with the onlyRole(SIGNING_ROLE) one as follows.

- function proposeTransaction(address to, uint256 value, bytes calldata data) external nonReentrant noneZeroAddress(to) onlyOwner returns (uint256) {
+ function proposeTransaction(address to, uint256 value, bytes calldata data) external nonReentrant noneZeroAddress(to) 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!