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

Signer revocation makes prior confirmations irrevocable, amplifying stale-approval risks

Author Revealed upon completion

Summary

When a signer’s role is revoked, they lose permission to call revokeConfirmation, even for confirmations they submitted while authorized. While this access control is individually reasonable, it causes confirmations to become irreversible after signer rotation, creating operational and governance risks and amplifying stale-approval execution issues.

Description

  • Normal behavior: Signers are allowed to revoke their own confirmations before a transaction is executed, providing a safety mechanism in case a signer changes their decision or a transaction is later deemed unsafe. This mechanism is intended to support safe governance and flexible decision-making in a multisig environment.

  • Issue: Once a signer’s role is revoked, they permanently lose the ability to revoke confirmations they previously submitted while authorized. Although this access control is individually logical, it causes prior confirmations to become irreversible after signer rotation, preventing safe cleanup of approvals and amplifying risks related to stale confirmations in changing signer sets.

function revokeConfirmation(uint256 txnId)
external
nonReentrant
transactionExists(txnId)
notExecuted(txnId)
@> onlyRole(SIGNING_ROLE)
{
_revokeConfirmation(txnId);
}

Risk

Likelihood:

  • Signer rotation (revoking/replacing signers) is a standard operational practice for multisigs (key rotation, compromised signer response, inactivity).

  • The issue occurs deterministically whenever a signer confirms a tx and is later revoked: revokeConfirmation becomes unavailable due to onlyRole(SIGNING_ROLE).

Impact:

  • This primarily affects governance/operational flexibility: approvals can become stuck and cannot be withdrawn by the original signer after rotation.

  • The security impact is mostly indirect and is strongest when combined with Finding #1 (stale confirmations counting toward execution).

Proof of Concept

Jaust paste in test/unit/MultiSigTimelockTest.t.sol

function test_NoRevokeConfirmationAfterRevokeSignerRole() public {
// Add 1 signer
multiSigTimelock.grantSigningRole(SIGNER_TWO);
​
// Propose tx
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
​
// Confirm tx
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
​
// Revoke signing role
vm.prank(OWNER);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
​
// Revoke confirmation
vm.prank(SIGNER_TWO);
vm.expectRevert();
multiSigTimelock.revokeConfirmation(txnId);
​
MultiSigTimelock.Transaction memory t = multiSigTimelock.getTransaction(txnId);
assertEq(t.confirmations, 1);
}

Recommended Mitigation

Allow an account that previously confirmed a transaction to revoke its own confirmation even after its signing role has been revoked. This preserves revokeConfirmation as a safety mechanism during signer rotation while still preventing non-signers from revoking arbitrary confirmations.

Concretely:

  • replace the external revokeConfirmation access gate from onlyRole(SIGNING_ROLE) to a custom check:

    • current signers can revoke (as before), and

    • former signers can revoke only if they had previously confirmed that transaction.

function revokeConfirmation(uint256 txnId)
external
nonReentrant
transactionExists(txnId)
notExecuted(txnId)
- onlyRole(SIGNING_ROLE)
{
+ // Allow either:
+ // - current signers (SIGNING_ROLE), or
+ // - former signers who previously confirmed this txn (can only revoke their own confirmation)
+ if (!hasRole(SIGNING_ROLE, msg.sender) && !s_signatures[txnId][msg.sender]) {
+ revert MultiSigTimelock__AccountIsNotASigner();
+ }
_revokeConfirmation(txnId);
}

Support

FAQs

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

Give us feedback!