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

revokeSigningRole Should Use revokeRole instead of _revokeRole

Author Revealed upon completion

revokeSigningRole Uses _revokeRole instead of revokeRole

Description

  • When creating a MultiSig, it is best practice to create an Admin role. This way, when revoking a signing role, you can make sure that the role revoking the signing role is the admin role, instead of solely relying on onlyOwner.

  • Using _revokeRole skips checks that make sure the msg.sender has the Admin role. It is much safer to give the owner Admin role and then use revokeRole.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account); // Should use revokeRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • This will occur every time the revokeSigningRole function is called by the owner.

Impact:

  • When using revokeRole, the MultiSig is properly secured, since it checks that the msg.sender is not only the owner but also the admin role.

  • Having role based authority over owner based authority could allow for transferral of ownership or the addition of more admin members


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");
​
...
​
/// @dev The constructor sets the deployer as the initial owner and first signer
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); // Set ADMIN_ROLE to a signers admin
+ _grantRole(ADMIN_ROLE, msg.sender);
_grantRole(SIGNING_ROLE, msg.sender);
}
​
...
​
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
- _revokeRole(SIGNING_ROLE, _account);
+ revokeRole(SIGNING_ROLE, _account);
}

Support

FAQs

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

Give us feedback!