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

Owner can be revoked from signers

Author Revealed upon completion

Owner can be revoked from signers

Description

The contract allows the owner to revoke their own signing role by calling the revokeSigningRole function with the owner’s address. This results in the owner losing signer privileges, which does not align with the intended access control model.

Risk

Likelihood: High
The owner can unintentionally revoke their own signing role.

Impact: Low
While the owner is removed from the signers list, this is not permanent. The owner can restore their signer status by granting themselves the signing role again.

Proof of Concept

The following unit test demonstrates that the owner successfully revokes their own signing role, reducing the signer count accordingly:

function test_revokeSigningRole_doesNotrevertForRevokingOwner() public {
assertEq(wallet.getSignerCount(), 1);
vm.startPrank(OWNER);
wallet.grantSigningRole(SIGNER_TWO);
assertEq(wallet.getSignerCount(), 2);
wallet.revokeSigningRole(OWNER);
assertEq(wallet.getSignerCount(), 1);
vm.stopPrank();
​
address[5] memory signers = wallet.getSigners();
assertEq(signers[0], SIGNER_TWO);
assertEq(signers[1], address(0));
assertEq(signers[2], address(0));
assertEq(signers[3], address(0));
assertEq(signers[4], address(0));
}

Mitigation

Prevent revocation of the owner’s signing role by adding a check in revokeSigningRole to revert when the target account is the owner:

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
​
+ if (_account == owner()) {
+ revert MultiSigTimelock__AccountIsOwner();
+ }
...
}

Support

FAQs

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

Give us feedback!