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

Contract Owner Can Revoke Their Own SIGNING_ROLE, Resulting in Functional Contradiction

Author Revealed upon completion

Contract Owner Can Revoke Their Own SIGNING_ROLE, Resulting in Functional Contradiction

Description

  • In the multi-signature wallet contract, the contract owner is automatically granted the SIGNING_ROLE and becomes the first signer.

  • Under normal circumstances, the owner should retain the signer identity at all times to participate in multi-signature operations.

  • However, the revokeSigningRole function allows the owner to remove their own SIGNING_ROLE, causing the owner to lose critical transaction operation capabilities.

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();
}
// @> Critical Issue: Missing special check for the owner address
// @> Allows _account == owner() to pass validation
// @> Enables the owner to remove their own signer role
// ... Array removal logic
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • Administrator misoperation or erroneous invocation of revokeSigningRole(owner) by automated scripts

  • Accidental revocation of self-permissions by the outgoing administrator during handover procedures

  • Social engineering attacks inducing administrators to execute self-revocation operations

Impact:

  • The owner loses eligibility to confirm transactions, revoke confirmations, and execute transactions, becoming unable to participate in the multi-signature process

  • The contract enters a "manageable but unusable" state, requiring re-authorization to restore normal functionality

  • Violates the design intent of the multi-signature wallet, conflicting with the dual-identity description of the administrator in documentation

Proof of Concept

  • Add the test_revokeSigningRole_Owner function to test/unit/MultiSigTimelockTest.t.sol as follows:

function test_revokeSigningRole_Owner() public grantSigningRoles {
// ARRANGE
vm.deal(OWNER, OWNER_BALANCE_ONE);
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
// Administrator misoperation: Revokes their own permissions
multiSigTimelock.revokeSigningRole(OWNER);
// ACT
vm.prank(OWNER);
vm.expectRevert();
multiSigTimelock.confirmTransaction(txnId);
}

  • Run the command in the console: forge test --mt test_revokeSigningRole_Owner -vv

Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest
[PASS] test_revokeSigningRole_Owner() (gas: 423109)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.78ms (244.70µs CPU time)

Recommended Mitigation

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__CannotRevokeOwner();
+ }
// Remaining original code...
}

Support

FAQs

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

Give us feedback!