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

Signer role can't call proposeTransaction()

Author Revealed upon completion

proposeTransaction has the onlyOwner modifier + Signer role can't call proposeTransaction()

Description

  • The proposeTransaction() function has the onlyOwner modifier.

  • A user with a signer role can't call proposeTransaction().

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

Risk

Likelihood:

  • A user with the signer role tries to call proposeTransaction().

Impact:

  • A user with the signer role can't call proposeTransaction().

Proof of Concept

  • SIGNER_TWO is granted the signer role by the admin.

  • SIGNER_TWO tries to call proposeTransaction() only to find that only the owner can call it.

// add this import to the beginning of test/unit/MultiSigTimelockTest.t.sol
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
​
// add this function in the MultiSigTimeLockTest conctract
function testSignerRoleCantProposeTransaction() public {
multiSigTimelock.grantSigningRole(SIGNER_TWO);
​
vm.prank(SIGNER_TWO);
vm.expectRevert(
abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, SIGNER_TWO)
);
​
multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
}
​

Test with:

forge test -vvv --match-test testSignerRoleCantProposeTransaction

Sample output:

Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest
[PASS] testSignerRoleCantProposeTransaction() (gas: 102504)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.94ms (163.20µs CPU time)
​
Ran 1 test suite in 13.64ms (2.94ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Recommended Mitigation

  • Add the modifier onlyOwnerOrSigners .

  • For the function proposeTransaction(), swap out the onlyOwner modifier with the onlyOwnerOrSigners modifier.

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