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

Revoked Signer's Confirmations Remain Active on Pending Transactions

Author Revealed upon completion

Root + Impact

Description

When revokeSigningRole() is called to remove a compromised or malicious signer, their existing confirmations on pending transactions remain active. The s_signatures[txnId][signer] mapping is never cleared, and the confirmations counter is never decremented. This allows revoked signers to contribute to transaction execution post-revocation.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
.
.
.
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
@> // MISSING: No cleanup of existing confirmations
@> // MISSING: No loop through s_signatures[txnId][_account]
@> // MISSING: No decrement of s_transactions[txnId].confirmations
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • Direct call of the protocol function revokeSigningRole() (standard flow).

Impact:

  • Compromised signers can help malicious transactions even after revocation

  • Security assumption broken: "only active signers can confirm"

  • Transactions may have fewer real confirmations than REQUIRED_CONFIRMATIONS

  • No way to audit which confirmations came from currently-active signers

Proof of Concept

Add this snipped of code to the MultiSigTimelockTest.t.sol test file.

function test_RevokeSignerCheckTransactionConfirmationRemains() public grantSigningRoles {
// ARRANGE
vm.deal(address(multiSigTimelock), OWNER_BALANCE_TWO);
​
// first transaction proposal
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_TWO, hex"");
​
// ACT
// get 3 / 5 confirmations for first transaction
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
​
vm.prank(OWNER);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
​
// ASSERT
MultiSigTimelock.Transaction memory txn = multiSigTimelock.getTransaction(txnId);
assertEq(txn.confirmations, 3, "Confirmations should remain 3 after revoking a signer");
}

How to execute:

forge test --mt test_RevokeSignerCheckTransactionConfirmationRemains -vvvv

Recommended Mitigation

Clear all pending confirmations from this signer

function revokeSigningRole(address _account) external nonReentrant onlyOwner {
// ... existing validation ...
+ // Clear all pending confirmations from this signer
+ for (uint256 txnId = 0; txnId < s_transactionCount; txnId++) {
+ if (!s_transactions[txnId].executed && s_signatures[txnId][_account]) {
+ s_signatures[txnId][_account] = false;
+ s_transactions[txnId].confirmations -= 1;
+ emit TransactionRevoked(txnId, _account);
+ }
+ }
// ... rest of function
}

Support

FAQs

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

Give us feedback!