MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

The owner can revoke their own signing role making them unable to sign new transactions

farnad

Root + Impact

Description

  • While the owner can revoke any users' signing role, he/she may mistakenly revoke their own one.

  • This way they will no longer be able to sign any new transactions unless they grant this role to themself again.

  • The owner's inclusion in the signers list is so important that in the documentation it is mentioned as "Up to 5 addresses in total (owner + up to 4 others) that possess the SIGNING_ROLE". Therefore, the owner is the fixed member and must ALWAYS have the role.

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();
}
​
@> //The owner check is missing here
​
// 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);
}

Risk

Likelihood:

  • Since the addresses usually are not easily relatable to individuals, it is fairly likely for the owner to mistakenly revoke their own address from the list of signers instead of someone else's.

Impact:

  • Even though as mentioned above, it is an invariant of the protocol, this issue does not break the functionality of the protocol because there should be enough other signers to sign the transactions without the owner's involvement.

Proof of Concept

Please add the following function to the test file and run it using the forge test --mt testOwnerCanRevokeTheirOwnSigningRole -vvvv command in the terminal.

function testOwnerCanRevokeTheirOwnSigningRole() public grantSigningRoles {
vm.prank(OWNER);
multiSigTimelock.revokeSigningRole(OWNER);
assertTrue(!multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), OWNER), "Owner still has signing role");
}

Recommended Mitigation

To solve this issue we just need to add the required check to the function.

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();
}
​
+ if (_account == owner()) {
+ revert MultiSigTimelock__InvalidAddress();
+ }
​
// 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);
}
Updates

Lead Judging Commences

kelechikizito Lead Judge 4 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Owner revokes her signing role

Support

FAQs

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

Give us feedback!