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

grantSigningRole Should Use grantRole Instead of _grantRole

Author Revealed upon completion

grantSigningRole uses _grantRole instead of grantRole

Description

  • It is best to create an Admin role when using MultiSig accounts. When granting a signing role, you can then check that the msg.sender is the admin account and has the authority to grant roles.

  • _grantRole skips admin role checks that make the MultiSig safer to use.

function grantSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsAlreadyASigner();
}
if (s_signerCount >= MAX_SIGNER_COUNT) {
revert MultiSigTimelock__MaximumSignersReached();
}
​
s_signers[s_signerCount] = _account;
s_isSigner[_account] = true;
s_signerCount += 1;
​
_grantRole(SIGNING_ROLE, _account); // Should use grantRole(SIGNING_ROLE, _account);
// emit SigningRoleGranted(_account); // commented this out because the inherited function(_grantRole) emits the event already
}

Risk

Likelihood:

  • This occurs every time the grantSigningRole function is called.

Impact:

  • The MultiSig is not properly secured simply by using owner based authentication. It is best practice when creating MultiSigs to create an admin role for extra security.

Recommended Mitigation

Make the deployer the admin role and signer role. Define the ADMIN_ROLE and then set the SIGNER_ROLE's admin to the ADMIN_ROLE.

bytes32 private constant SIGNING_ROLE = keccak256("SIGNING_ROLE");
+ /// @dev The role identifier for the admin role
+ bytes32 private constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
​
...
​
constructor() Ownable(msg.sender) {
// Automatically add deployer as first signer
s_signers[0] = msg.sender;
s_isSigner[msg.sender] = true;
s_signerCount = 1;
​
// Grant signing role to deployer
+ _setRoleAdmin(SIGNING_ROLE, ADMIN_ROLE);
+ _grantRole(ADMIN_ROLE, msg.sender);
_grantRole(SIGNING_ROLE, msg.sender);
}
​
...
​
function grantSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsAlreadyASigner();
}
if (s_signerCount >= MAX_SIGNER_COUNT) {
revert MultiSigTimelock__MaximumSignersReached();
}
​
s_signers[s_signerCount] = _account;
s_isSigner[_account] = true;
s_signerCount += 1;
​
- _grantRole(SIGNING_ROLE, _account);
+ grantRole(SIGNING_ROLE, _account);
// emit SigningRoleGranted(_account); // commented this out because the inherited function(_grantRole) emits the event already
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!