Root + Impact
Permanent Loss of Funds. Once the signer count drops below 3, no transaction (including those to send ETH, call external contracts, or even grantSigningRole to fix the state) can be executed. Any ETH stored in the contract is permanently trapped.
Description
-
The MultiSigTimelock contract has a hardcoded quorum requirement defined by REQUIRED_CONFIRMATIONS = 3. However, the revokeSigningRole function only prevents the owner from revoking the last signer.
-
Because this check does not account for the REQUIRED_CONFIRMATIONS constant, the owner can successfully revoke signers until only 2 remain. Since a transaction requires 3 confirmations to execute, and there are only 2 signers left in the system, it becomes mathematically impossible to reach quorum.This results in a permanent Deadlock of the contract.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
@> if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
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
mpact:
Permanent Loss of Funds. Once the signer count drops below 3, no transaction (including those to send ETH, call external contracts, or even grantSigningRole to fix the state) can be executed. Any ETH stored in the contract is permanently trapped.
Proof of Concept
This POC prove that the OWNER can reduce the signer count to 2, at which point the wallet becomes unusable.
1. Confirm starting state
2. OWNER revokes SIGNER_THREE
3. Prove the invariant is broken
4. Prove execution is impossible
Add below code into MultiSigTimelockTest.t.sol
pragma solidity ^0.8.19;
import {Test, console2} from "forge-std/Test.sol";
import {MultiSigTimelock} from "src/MultiSigTimelock.sol";
import {EthRejector} from "test/utils/EthRejector.sol";
import {TestTimelockDelay} from "test/utils/TestTimelockDelay.sol";
import {DeployMultiSigTimelock} from "script/DeployMultiSigTimelock.s.sol";
import {GrantSigningRole} from "script/GrantSigningRole.s.sol";
import {ProposeTransactionScript} from "script/Interact.s.sol";
import {ConfirmTransactionScript} from "script/Interact.s.sol";
import {ExecuteTransactionScript} from "script/Interact.s.sol";
contract MultiSigTimeLockTest is Test {
MultiSigTimelock multiSigTimelock;
EthRejector ethRejector;
TestTimelockDelay testTimelockDelay;
DeployMultiSigTimelock deployer;
GrantSigningRole grantor;
address public OWNER = address(this);
address public SIGNER_TWO = makeAddr("signer_two");
address public SIGNER_THREE = makeAddr("signer_three");
address public SIGNER_FOUR = makeAddr("signer_four");
address public SIGNER_FIVE = makeAddr("signer_five");
address public SPENDER_ONE = makeAddr("spender_one");
address public SPENDER_TWO = makeAddr("spender_two");
function setUp() public {
multiSigTimelock = new MultiSigTimelock();
ethRejector = new EthRejector();
vm.deal(address(multiSigTimelock), 100 ether);
}
function test_Audit_LivenessInvariantBroken() public {
address[] memory signersArray = new address[](5);
signersArray[0] = OWNER;
multiSigTimelock.grantSigningRole(SIGNER_TWO);
signersArray[1] = SIGNER_TWO;
multiSigTimelock.grantSigningRole(SIGNER_THREE);
signersArray[2] = SIGNER_THREE;
assertEq(multiSigTimelock.getSignerCount(), 3);
assertEq(multiSigTimelock.getRequiredConfirmations(), 3);
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
uint256 totalSigners = multiSigTimelock.getSignerCount();
uint256 threshold = multiSigTimelock.getRequiredConfirmations();
console2.log("signers count after revocation: ", totalSigners);
console2.log("Minimum Threshold required: ", threshold);
assertTrue(totalSigners < threshold, "Total number of signers is less than required threshold");
uint256 txId = multiSigTimelock.proposeTransaction(address(ethRejector), 0.5 ether, "");
multiSigTimelock.confirmTransaction(txId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txId);
vm.expectRevert();
multiSigTimelock.executeTransaction(txId);
console2.log("Result: Transaction Failed. 100 ether is Permanently locked.");
}
}
Recommended Mitigation
Update the revokeSigningRole function to prevent the signer count from ever falling below the required execution threshold.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
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();
- }
+ if (s_signerCount <= REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__CannotRevokeBelowQuorum();
+ }
// 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;
_revokeRole(SIGNING_ROLE, _account);
}
