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

Signers with `SIGNING_ROLE` cannot propose new transactions

Author Revealed upon completion

Impact: H

Likelihood: H

Root + Impact

Description

  • The protocol's intended functionality is violated because the signers with SIGNING_ROLE cannot propose any transactions.

  • There is onlyOwner modifier set to proposeTransaction function, which means that the permission is tied to the owner, not to the role, so the signer with SIGNING_ROLE can not propose a transaction.

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

Risk

Likelihood:

  • The issue occurs when a signer tries to propose a transaction using MultiSigTimelock::proposeTransaction function

Impact:

  • The protocol's intended functionality is violated because the signers with SIGNING_ROLE cannot propose any transactions.

Proof of Concept

Please, add the following test_fuzz_signerCanNot_ProposeTransaction to MultiSigTimeLockTest.sol - the attempt to propose a transaction by a signer with a granted role is reverted:

​
function test_fuzz_signerCanNot_ProposeTransaction(uint256 addressSeed, uint256 amount) public {
// ARRANGE
amount = bound(amount, 1, type(uint96).max);
​
addressSeed = bound(addressSeed, 1, type(uint160).max);
address signer = address(uint160(addressSeed));
address spender = address(uint160(vm.randomUint(1, type(uint160).max)));
​
multiSigTimelock.grantSigningRole(signer);
assertEq(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), signer), true);
​
// ACT
vm.prank(signer);
vm.expectPartialRevert(Ownable.OwnableUnauthorizedAccount.selector);
multiSigTimelock.proposeTransaction(spender, amount, hex"");
}
​

Recommended Mitigation

  1. Add already existing, but commented out for some reason, modifier onlySigners and add an error MultiSigTimelock__NotASigner for it. Alternatively, use onlyRole(SIGNING_ROLE) modifier from Openzeppelin's AccessControl.sol.

  2. Remove onlyOwner modifier and add onlySigners modifier to the proposeTransaction;

+ error MultiSigTimelock__NotASigner();
​
+ modifier onlySigners() {
+ if (!hasRole(SIGNING_ROLE, msg.sender)) {
+ revert MultiSigTimelock__NotASigner();
+ }
+ _;
+ }
​
function proposeTransaction(address to, uint256 value, bytes calldata data)
external
nonReentrant
noneZeroAddress(to)
- onlyOwner
+ onlySigners
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!