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

The signer count can drop below confirmation threshold

Author Revealed upon completion

Root + Impact

Description

  • In a normal scenario a multisig wallet should maintain enough signers to meet the confirmation threshold

  • The problem is that the revokeSigningRole() function only prevents reducing signers to zero by checking s_signerCount <= 1, but does not enforce that s_signerCount >= REQUIRED_CONFIRMATIONS (3). This allows the owner to revoke signers down to 2 or even 1 active signer

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
@> // Only prevents removing the last signer, not dropping below 3
@> if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
// Owner can revoke down to 1 signer, but REQUIRED_CONFIRMATIONS = 3
// ...
}
​
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
@> if (txn.confirmations < REQUIRED_CONFIRMATIONS) { // Always requires 3
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
// Cannot execute if only 2 or 1 signers exist
}

Risk

Likelihood:

  • Owner may revoke too many signers

  • Reason 2

Impact:

  • Multisig becomes non functional

  • Funds permanently locked if there are less than 3 signers

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {MultiSigTimelock} from "./MultiSigTimelock.sol";
​
contract SignerThresholdExploit {
function testSignerBelowThreshold() external {
MultiSigTimelock wallet = new MultiSigTimelock();
payable(address(wallet)).transfer(100 ether);
address signer1 = address(0x1);
address signer2 = address(0x2);
address signer3 = address(0x3);
address signer4 = address(0x4);
wallet.grantSigningRole(signer1);
wallet.grantSigningRole(signer2);
wallet.grantSigningRole(signer3);
wallet.grantSigningRole(signer4);
// Owner revokes 3 signers, leaving only 2 total
wallet.revokeSigningRole(signer2);
wallet.revokeSigningRole(signer3);
wallet.revokeSigningRole(signer4);
assert(wallet.getSignerCount() == 2);
​
uint256 txId = wallet.proposeTransaction(address(0x9999), 1 ether, "");
wallet.confirmTransaction(txId);
vm.prank(signer1);
wallet.confirmTransaction(txId);
// Only 2 confirmations possible, need 3
MultiSigTimelock.Transaction memory txn = wallet.getTransaction(txId);
assert(txn.confirmations == 2);
// Execution will always revert
vm.expectRevert();
wallet.executeTransaction(txId);
​
}
}

Recommended Mitigation

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) {
+ // Prevent reducing signers below the required confirmation threshold
+ if (s_signerCount <= REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max;
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) {
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);
}

Support

FAQs

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

Give us feedback!