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

If the number of signers is less than REQUIRED_CONFIRMATIONS after the signer is removed, multisignature transactions will be blocked.

Author Revealed upon completion

Root + Impact

Description

  • Assume the current state:

    There are 3 signers: Alice, Bob, and Charlie

    REQUIRED_CONFIRMATIONS = 3

    Transaction #1 was initiated by Alice, and Bob and Charlie have confirmed it.

    At this point, the owner calls:

    revokeSigningRole(Alice);

    Result:

    The number of signers changes from 3 to 2;

    However, REQUIRED_CONFIRMATIONS remains 3.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
} // Only check if the number of signers is greater than 1, not greater than or equal to REQUIRED_CONFIRMATIONS.
​
uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • If some signers are removed, causing the signer count to fall below REQUIRED_CONFIRMATIONS, the system enters a non-functional state.

Impact:

  • All transactions carry the risk of not being able to be executed (confirmations are never sufficient) unless the number of signers is increased.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "forge-std/Test.sol";
import "../src/MultiSigTimelock.sol";
​
contract MultiSigTimelockDeadlockTest is Test {
MultiSigTimelock multiSig;
​
address owner = address(0xA11CE);
address alice = address(0xB0B);
address bob = address(0xC0C);
address charlie = address(0xD0D);
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), 10 ether);
}
​
function testDeadlockOccursWhenRemovingTooManySigners() public {
// 1. owner proposes
vm.prank(owner);
uint256 txId = multiSig.proposeTransaction(recipient, 1 ether, "");
​
// 2. Alice & Bob confirm
vm.prank(alice);
multiSig.confirmTransaction(txId);
vm.prank(bob);
multiSig.confirmTransaction(txId);
​
// 3. Remove Charlie (now signer count < required confirmations)
vm.prank(owner);
multiSig.revokeSigningRole(charlie);
​
// 4. Try to execute (should revert forever)
vm.expectRevert(); // insufficient confirmations (3 required, only 2 exist)
vm.prank(bob);
multiSig.executeTransaction(txId);
}
}

Recommended Mitigation

function revokeSigningRole(address _account) external onlyOwner {
if (s_signerCount <= REQUIRED_CONFIRMATIONS) {
revert("Cannot remove signer: would drop below required confirmations");
}
...
}

Support

FAQs

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

Give us feedback!