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

Revoked signer confirmations are valid

Author Revealed upon completion

Root + Impact

Description

  • When a signer is revoked their past confirmations should be invalidated

  • The problem is that the revokeSigningRole() function removes the signer from the active signer set but does not clear their confirmations from s_signatures mapping or decrement the confirmations counter

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// ... validation ...
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
@> // No cleanup of s_signatures[txnId][_account] for all pending transactions
@> // No decrement of Transaction.confirmations for transactions they signed
}
​
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
@> if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
// ... execution continues with stale confirmations
}

Risk

Likelihood:

  • Revoking the signer is critical, but it doesn't work

  • Reason 2

Impact:

  • Signer confirmations persist afeter removal

  • Can't stop malicious transactions

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {MultiSigTimelock} from "./MultiSigTimelock.sol";
​
contract RevokedSignerExploit {
function testRevokedSignerConfirmationPersists() external {
MultiSigTimelock wallet = new MultiSigTimelock();
payable(address(wallet)).transfer(100 ether);
address signer1 = address(0x1);
address signer2 = address(0x2);
address malicious = address(0x3);
wallet.grantSigningRole(signer1);
wallet.grantSigningRole(signer2);
wallet.grantSigningRole(malicious);
// Malicious signer proposes and confirms transaction
uint256 txId = wallet.proposeTransaction(address(0x9999), 100 ether, "");
vm.prank(signer1);
wallet.confirmTransaction(txId);
vm.prank(malicious);
wallet.confirmTransaction(txId);
vm.prank(signer2);
wallet.confirmTransaction(txId);
wallet.revokeSigningRole(malicious);
​
MultiSigTimelock.Transaction memory txn = wallet.getTransaction(txId);
assert(txn.confirmations == 3);
// Wait for timelock
vm.warp(block.timestamp + 7 days + 1);
// Any remaining signer can execute with stale confirmations
vm.prank(signer1);
wallet.executeTransaction(txId);
assert(address(0x9999).balance == 100 ether);
}
}

Recommended Mitigation

function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
- // CHECKS
- // 1. Check if enough confirmations
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
- revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
- }
+ // CHECKS
+ // 1. Recompute confirmations from current active signers only
+ uint256 validConfirmations = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ if (s_signatures[txnId][s_signers[i]]) {
+ validConfirmations++;
+ }
+ }
+
+ if (validConfirmations < REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, validConfirmations);
+ }
+
// 2. Check if timelock period has passed
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
if (txn.value > address(this).balance) {
revert MultiSigTimelock__InsufficientBalance(address(this).balance);
}
​
// EFFECTS
// 3. Mark as executed BEFORE the external call (prevent reentrancy)
txn.executed = true;
​
// INTERACTIONS
// 4. Execute the transaction
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) {
revert MultiSigTimelock__ExecutionFailed();
}
​
// 5. Emit eventt
emit TransactionExecuted(txnId, txn.to, txn.value);
}

Support

FAQs

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

Give us feedback!