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

The removed signer will still be counted in confirmations for previously confirmed transactions.

Author Revealed upon completion

Root + Impact

Description

  • When removing a signer:

    s_isSigner[_account] = false;

    does not clear the address's signature status in past transactions.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
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;//does not clear the address's signature status in past transactions.
_revokeRole(SIGNING_ROLE, _account);
}
​

Risk

Likelihood:

  • Assumptions:

    Initial signers: Alice, Bob, Charlie

    REQUIRED_CONFIRMATIONS = 3

    Alice initiates transaction #1, which Bob and Charlie have signed but not yet executed.

    At this point, the owner calls:

    revokeSigningRole(Alice);

    A new signer is added:

    grantSigningRole(David);

    Vulnerability occurs:

    Transaction #1 still has 2 confirmations (Bob, Charlie).

    Now, it only needs to be signed again by the new signer David, bringing the confirmation count to 3, to execute!

    Even if Alice is removed, her confirmation remains valid indefinitely.

    An attacker can exploit this method to prematurely confirm sensitive transactions by rotating signers.

Impact:

  • Abuse of transaction authority: The "old signature" of a revoked signer can still drive transaction execution;

  • Governance insecurity: This undermines the credibility of multi-signature.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "forge-std/Test.sol";
import "../src/MultiSigTimelock.sol";
​
contract MultiSigTimelockSignerRevokeBugTest is Test {
MultiSigTimelock multiSig;
​
address owner = address(0xA11CE);
address alice = address(0xB0B);
address bob = address(0xC0C);
address charlie = address(0xD0D);
address david = address(0xE0E);
address recipient = address(0xF0F);
​
function setUp() public {
vm.prank(owner);
multiSig = new MultiSigTimelock();
​
vm.startPrank(owner);
multiSig.grantSigningRole(alice);
multiSig.grantSigningRole(bob);
multiSig.grantSigningRole(charlie);
vm.stopPrank();
​
vm.deal(address(multiSig), 100 ether);
}
​
function testSignerRevokeDoesNotClearOldConfirmations() public {
// Step 1: owner propose transaction
vm.prank(owner);
uint256 txId = multiSig.proposeTransaction(recipient, 1 ether, "");
​
// Step 2: Alice, Bob, Charlie confirm
vm.prank(alice);
multiSig.confirmTransaction(txId);
vm.prank(bob);
multiSig.confirmTransaction(txId);
​
// Step 3: revoke Alice
vm.prank(owner);
multiSig.revokeSigningRole(alice);
​
// Step 4: add David
vm.prank(owner);
multiSig.grantSigningRole(david);
​
// Step 5: new signer David confirms, total confirmations = 3 (Alice’s old one still counts)
vm.prank(david);
multiSig.confirmTransaction(txId);
​
// Step 6: transaction executes successfully
vm.prank(bob);
multiSig.executeTransaction(txId);
​
assertEq(recipient.balance, 1 ether, "Recipient should have received funds");
}
}

Recommended Mitigation

function _executeTransaction(uint256 txnId) internal {
...
// // Add: Re-verify whether the confirmer is still a valid signer
uint256 validConfirmations = 0;
for (uint256 i = 0; i < s_signerCount; i++) {
address signer = s_signers[i];
if (signer != address(0) && s_signatures[txnId][signer]) {
validConfirmations++;
}
}
if (validConfirmations < REQUIRED_CONFIRMATIONS) {
revert("Not enough active signers");
}
...
}

Support

FAQs

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

Give us feedback!