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

Revoked Signers Count as Valid Confirmations (Ghost Vote)

Author Revealed upon completion

Root + Impact

Description

When a signer is revoked using revokeSigningRole, their previous confirmations on pending transactions are not removed. The check in executeTransaction only verifies txn.confirmations >= REQUIRED_CONFIRMATIONS (a simple counter) and does NOT verify if the addresses that provided those confirmations are still valid signers.

This means a compromised signer can:

  1. Approve a malicious transaction.

  2. Be detected and revoked by the Owner.

  3. The malicious transaction still retains 1 valid confirmation count.

  4. The remaining valid signers can then (intentionally or accidentally) complete the quorum with fewer actual valid signatures than intended.

For example, if REQUIRED = 3, and Malicious_A signs (count=1), then is revoked. New signer D is added. If B and C sign, count=3. The tx executes. Effectively, B and C executed a transaction with only 2 valid concurrent signatures, relying on the "ghost vote" of revoked A.

Combined with the lack of a cancelTransaction function (See I-1), this makes it impossible for the Owner to stop a pending malicious transaction that has already gathered confirmations, even if they replace the entire signer set. The past votes haunt the system.

/// File: src/MultiSigTimelock.sol:209
function revokeSigningRole(address _account) ... {
// ...
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
// @> MISSING: Cleanup of s_signatures or decrement of count for pending txs
}
​
/// File: src/MultiSigTimelock.sol:355
if (txn.confirmations < REQUIRED_CONFIRMATIONS) { ... }
// @> Logic checks counter, not identity validity

Risk

Likelihood: Medium (Requires revocation and pending tx).
Impact: High (Weakens the security model, allows revoked keys to influence future executions, unstoppable transactions).

Proof of Concept

  1. Signer A confirms tx. confirmations = 1.

  2. Owner revokes A.

  3. Owner adds D.

  4. B and C confirm. confirmations = 3.

  5. executeTransaction succeeds.

  6. Result: Transaction executed with only 2 currently valid signers (B and C).

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {Test, console} from "forge-std/Test.sol";
import {MultiSigTimelock} from "../../src/MultiSigTimelock.sol";
​
contract TestGhostVotes is Test {
MultiSigTimelock public multiSig;
address public deployer;
address public alice;
address public bob;
address public charlie;
address public david;
​
function setUp() public {
deployer = makeAddr("deployer");
alice = makeAddr("alice");
bob = makeAddr("bob");
charlie = makeAddr("charlie");
david = makeAddr("david");
​
vm.prank(deployer);
multiSig = new MultiSigTimelock();
}
​
function test_GhostVote_RevokedSignerCounts() public {
// 1. Setup Initial Signers (Deployer + Alice + Bob) -> Count = 3
vm.startPrank(deployer);
multiSig.grantSigningRole(alice);
multiSig.grantSigningRole(bob);
// 2. Propose Transaction
uint256 txId = multiSig.proposeTransaction(address(0x123), 0.5 ether, "");
vm.stopPrank();
​
// 3. Alice (Signer) Confirms
vm.prank(alice);
multiSig.confirmTransaction(txId);
assertEq(multiSig.getTransaction(txId).confirmations, 1);
​
// 4. Revoke Alice
// Alice is compromised or removed.
vm.prank(deployer);
multiSig.revokeSigningRole(alice); // Now signers = 2 (Deployer, Bob)
​
// 5. Add Charlie to maintain quorum size capability
vm.prank(deployer);
multiSig.grantSigningRole(charlie); // Signers = 3 (Deployer, Bob, Charlie)
​
// 6. Deployer and Bob confirm
vm.prank(deployer);
multiSig.confirmTransaction(txId);
vm.prank(bob);
multiSig.confirmTransaction(txId);
​
// Confirmations = 3 (Alice + Deployer + Bob)
// Note: Charlie has NOT signed.
// Active signers who signed: Deployer + Bob = 2.
// But confirmations count says 3.
assertEq(multiSig.getTransaction(txId).confirmations, 3);
​
// 7. Execute - Should Succeed with Ghost Vote
vm.deal(address(multiSig), 1 ether);
vm.prank(deployer);
multiSig.executeTransaction(txId);
​
// If successful, test passes
assertTrue(multiSig.getTransaction(txId).executed);
}
}

Recommended Mitigation

Recommendation: Create a version number for the signer set, or simply accept that executeTransaction must re-validate signers. Since iterating 5 signers is cheap, executeTransaction should loop through s_signers and count how many have s_signatures[txnId][signer] == true.

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
+ uint256 validConfirmations = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ if (s_signatures[txnId][s_signers[i]]) {
+ validConfirmations++;
+ }
+ }
+ if (validConfirmations < REQUIRED_CONFIRMATIONS) revert ...
​
// ...
}

Support

FAQs

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

Give us feedback!