MultiSigTimelock Security Review

Prepared by: Khant Wai Yan Aung (SnavOhBurmaa)

Lead Auditors: Khant Wai Yan Aung (SnavOhBurmaa)

date: September 6, 2026

See table

MultiSigTimelock Security Review
Table of contents
About Me
Disclaimer
Risk Classification
Audit Details
Scope
Protocol Summary
Roles
Executive Summary
Issues found
Findings

About Me

I'm a smart contract auditor focus on EVM and Solidity protocols. This review was carried out as independent security practice on the Timelock Multi-Signature Wallet codebase.

Disclaimer

I made every effort to find as many vulnerabilities as possible

Risk Classification

		Impact
		High	Medium	Low
	High	H	H/M	M
Likelihood	Medium	H/M	M	M/L
	Low	M	M/L	L

Audit Details

The project is the Timelock Multi-Signature Wallet, one contract of 205 nSLOC. The reviewed version uses pragma solidity ^0.8.19 and builds against OpenZeppelin v5.4.0. The reviewer is Khant Wai Yan Aung (SnavOhBurmaa) and the date is September 6, 2026. Tools used were manual review, Foundry (forge) for the PoC, Slither 0.11.5 and Aderyn 0.6.8.

Scope

src/
  MultiSigTimelock.sol

Protocol Summary

MultiSigTimelock is an ETH wallet controlled by up to five signers. Any action needs three of them to agree. It is role based, not signature based: signers confirm on chain by calling the contract, and the contract counts the confirmations.

The flow has three steps. The owner proposes a transaction with a target, an ETH value and optional calldata. Signers confirm it, and can take their confirmation back. Once three confirmations are in and the timelock has passed, any signer executes it and the wallet makes the call.

The timelock depends on the ETH value. Under 1 ETH there is no delay. From 1 ETH there is one day, from 10 ETH two days, and from 100 ETH seven days. The idea is that small payments are quick while large ones give the signers time to notice and react.

The wallet accepts ETH through receive. It can also hold tokens and call other contracts through the calldata field.

Roles

The owner is the deployer. It is the only account that can propose transactions and the only one that can add or remove signers. It is also the first signer.

Signers hold SIGNING_ROLE. They confirm, revoke their own confirmation, and execute. There are at most five, and three must agree.

Anyone can send ETH to the wallet.

The README says signers can propose and that the owner holds DEFAULT_ADMIN_ROLE. Neither is true in the code. Only the owner can propose, and no account has the admin role. The findings below follow the code.

Executive Summary

The wallet is built on two ideas, that three people must agree and that big transfers must wait. Neither holds up.

The owner controls who the signers are with no vote from anyone else, so it can fill the signer list with its own keys and reach three confirmations alone. The multisig is really one key. The timelock only reads the ETH value, so a token transfer, a contract call, or a large payment split into pieces under 1 ETH all skip the wait completely.

Beneath that, the delay counts from the proposal rather than from the third confirmation, a removed signer's vote still counts, a signer can quietly drop its role through OpenZeppelin's renounceRole, and there is no way to cancel or expire a proposal. Slither and Aderyn only surfaced low level items such as the floating pragma and ignored return values.

The code is clean and well commented, and reentrancy and the CEI order in execution are handled correctly. The problems are in the design of who has power and what the timelock actually measures.

Issues found

Severity	Number of issues found
High	2
Medium	4
Low	5
Total	11

ID	Title	Severity
[H-1]	The owner alone can control the "3 of 5" multisig	High
[H-2]	The timelock only looks at ETH value, so token transfers and any contract call skip it	High
[M-1]	A revoked signer's confirmation still counts	Medium
[M-2]	The timelock starts at proposal time, not when the transaction is approved	Medium
[M-3]	A signer can call `renounceRole` and leave the signer list out of sync	Medium
[M-4]	Proposed transactions never expire and cannot be cancelled	Medium
[L-1]	Comment says the first signer cannot be removed, but the code only protects the last one	Low
[L-2]	Ownership transfer is one step	Low
[L-3]	Floating pragma	Low
[L-4]	Return value of `_grantRole` and `_revokeRole` is ignored	Low
[L-5]	Two sources of truth for who is a signer	Low

Findings

High

[H-1] The owner alone can control the "3 of 5" multisig

Description:

The owner is the only account that can add signers, and there is no check on who those signers are. So the owner can add two more keys it owns, propose a transaction, confirm it three times with its own keys and execute it. The other signers never need to agree. The multisig only protects the funds if the owner is honest and the owner key is safe, which makes it a 1 of 1 wallet in practice.

// src/MultiSigTimelock.sol:187
function grantSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
    ...
    s_signers[s_signerCount] = _account;
    s_isSigner[_account] = true;
    s_signerCount += 1;
    _grantRole(SIGNING_ROLE, _account);
}

The owner can also revokeSigningRole on any honest signer at any time, so honest signers cannot even block a bad transaction by refusing to sign, the owner just swaps them out. On top of that, Ownable from OpenZeppelin lets the owner call renounceOwnership. If that ever happens nobody can propose or manage signers again and every ETH and token in the wallet is stuck forever.

Impact:

Likelihood High

Risk High

Proof of Concept:

function test_H1_OwnerAloneControlsWallet() public {
    address ownerKey2 = makeAddr("ownerKey2");
    address ownerKey3 = makeAddr("ownerKey3");

    vm.startPrank(owner);
    wallet.grantSigningRole(ownerKey2);
    wallet.grantSigningRole(ownerKey3);
    uint256 id = wallet.proposeTransaction(attacker, 0.9 ether, "");
    wallet.confirmTransaction(id);
    vm.stopPrank();
    vm.prank(ownerKey2); wallet.confirmTransaction(id);
    vm.prank(ownerKey3); wallet.confirmTransaction(id);
    vm.prank(owner); wallet.executeTransaction(id);

    assertEq(attacker.balance, 0.9 ether);
}

[PASS] test_H1_OwnerAloneControlsWallet()
Logs:
  signers      : 3
  attacker got : 900000000000000000

Recommended Mitigation:

Adding or removing a signer should itself go through the multisig, not through onlyOwner. Make grantSigningRole and revokeSigningRole callable only by the wallet itself, so they need 3 confirmations like any other action. Let any signer propose, not just the owner. Block renounceOwnership so the wallet can never be locked by mistake. Because signers can then no longer be added by one person after deploy, the constructor should take the starting signer list and refuse to deploy with fewer than three.

[H-2] The timelock only looks at ETH value, so token transfers and any contract call skip it

Description:

_getTimelockDelay picks the delay from txn.value only. A transaction that moves ERC20 tokens, changes ownership of another contract, or approves a spender has value = 0, so it gets no delay at all, no matter how much it is worth.

// src/MultiSigTimelock.sol:409
function _getTimelockDelay(uint256 value) internal pure returns (uint256) {
    ...
    if (value >= sevenDaysTimeDelayAmount) return SEVEN_DAYS_TIME_DELAY;
    ...
    return NO_TIME_DELAY;
}

The same idea works for plain ETH. Nothing stops someone from splitting one 100 ETH payment into many payments under 1 ETH. Each one has zero delay, so the 7 day window that is supposed to give people time to react never happens.

Impact:

Likelihood High

Risk High

Proof of Concept:

Token case. The wallet holds 1,000,000 tokens and they leave in the same block they are approved.

function test_H2_TokenTransferHasNoTimelock() public {
    MockToken token = new MockToken(address(wallet), 1_000_000e18);
    vm.startPrank(owner);
    wallet.grantSigningRole(alice);
    wallet.grantSigningRole(bob);
    bytes memory data = abi.encodeWithSignature("transfer(address,uint256)", attacker, 1_000_000e18);
    uint256 id = wallet.proposeTransaction(address(token), 0, data);
    wallet.confirmTransaction(id);
    vm.stopPrank();
    vm.prank(alice); wallet.confirmTransaction(id);
    vm.prank(bob); wallet.confirmTransaction(id);

    // no vm.warp, same block
    vm.prank(owner); wallet.executeTransaction(id);
    assertEq(token.balanceOf(attacker), 1_000_000e18);
}

Split case. 101 payments of 0.99 ETH move about 100 ETH with zero delay.

[PASS] test_H2_TokenTransferHasNoTimelock()
Logs:
  attacker tokens: 1000000

[PASS] test_H2_SplitLargeTransfer()
Logs:
  attacker eth  : 99
  time passed   : 0 seconds

Recommended Mitigation:

Give every transaction at least a 1 day delay and only use the value tiers to make it longer. Treat any transaction with non empty data as a large one, since the wallet cannot know what a contract call is worth. With a floor of 1 day, splitting 100 ETH into 101 pieces no longer skips the wait.

-    uint256 private constant NO_TIME_DELAY = 0;
+    uint256 private constant MIN_TIME_DELAY = 24 hours;

-    function _getTimelockDelay(uint256 value) internal pure returns (uint256) {
+    function _getTimelockDelay(uint256 value, bytes memory data) internal pure returns (uint256) {
         uint256 sevenDaysTimeDelayAmount = 100 ether;
         uint256 twoDaysTimeDelayAmount = 10 ether;
-        uint256 oneDayTimeDelayAmount = 1 ether;

-        if (value >= sevenDaysTimeDelayAmount) {
+        // Any contract call (tokens, approvals, ownership changes) is treated as a large transfer
+        if (data.length > 0 || value >= sevenDaysTimeDelayAmount) {
             return SEVEN_DAYS_TIME_DELAY;
         } else if (value >= twoDaysTimeDelayAmount) {
             return TWO_DAYS_TIME_DELAY;
-        } else if (value >= oneDayTimeDelayAmount) {
-            return ONE_DAY_TIME_DELAY;
         } else {
-            return NO_TIME_DELAY;
+            // Every transaction waits at least one day, so splitting a payment does not skip the delay
+            return MIN_TIME_DELAY;
         }
     }

     // in _executeTransaction
-        uint256 requiredDelay = _getTimelockDelay(txn.value);
+        uint256 requiredDelay = _getTimelockDelay(txn.value, txn.data);

If a stricter rule is wanted, keep a running total of ETH sent in the last 7 days and use that total, not the single transaction value, to pick the tier.

Medium

[M-1] A revoked signer's confirmation still counts

Description:

revokeSigningRole removes the signer from the list and the role, but it does not touch s_signatures or confirmations on open transactions. If a signer key is stolen and the owner removes it, every transaction that key already confirmed still carries that vote. The removed signer also cannot call revokeConfirmation any more because it lost the role, so the vote is frozen in.

// src/MultiSigTimelock.sol:235-239
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
// nothing about the confirmations this account already gave

Impact:

Likelihood Medium

Risk Medium

Proof of Concept:

function test_M1_RevokedSignerConfirmationStillCounts() public {
    ...
    vm.prank(alice); wallet.confirmTransaction(id);
    vm.prank(bob); wallet.confirmTransaction(id);

    // bob's key is found to be compromised, owner removes him
    vm.prank(owner); wallet.revokeSigningRole(bob);

    // still executes with only 2 live signers
    vm.prank(owner); wallet.executeTransaction(id);
    assertEq(attacker.balance, 0.5 ether);
}

[PASS] test_M1_RevokedSignerConfirmationStillCounts()
Logs:
  signers now    : 2
  confirmations  : 3

Recommended Mitigation:

Count confirmations at execution time instead of trusting a stored counter. Loop over the current s_signers and count how many have signed. A removed signer's vote then drops out on its own. The array is at most 5 long, so the loop is cheap.

+    /// @dev Count confirmations only from accounts that are still signers right now.
+    function _liveConfirmations(uint256 txnId) internal view returns (uint256 count) {
+        for (uint256 i = 0; i < s_signerCount; i++) {
+            if (s_signatures[txnId][s_signers[i]]) {
+                count++;
+            }
+        }
+    }

     // in _executeTransaction
-        if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
-            revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
+        uint256 live = _liveConfirmations(txnId);
+        if (live < REQUIRED_CONFIRMATIONS) {
+            revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, live);
         }

Also stop the wallet from removing so many signers that it can never execute again:

     // in revokeSigningRole
-        if (s_signerCount <= 1) {
+        if (s_signerCount <= REQUIRED_CONFIRMATIONS) {
             revert MultiSigTimelock__CannotRevokeLastSigner();
         }

[M-2] The timelock starts at proposal time, not when the transaction is approved

Description:

executionTime is proposedAt + delay. A proposal can sit with zero confirmations for 7 days while nobody pays attention to it, then get its three confirmations and be executed in the same block. The delay is meant to give people time to review an approved transaction, but here the review window can be over before anyone has approved.

// src/MultiSigTimelock.sol:359-361
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {

Impact:

Likelihood Medium

Risk Medium

Proof of Concept:

[PASS] test_M2_TimelockRunsBeforeApproval()
Logs:
  attacker eth   : 500

500 ETH leaves the wallet in the same block as the third confirmation, because the proposal was made 7 days earlier.

Recommended Mitigation:

Store the time the transaction reached REQUIRED_CONFIRMATIONS and count the delay from there. Reset it if a revoke drops the count under the threshold.

+    error MultiSigTimelock__TransactionNotApproved();

     struct Transaction {
         ...
         uint256 proposedAt;
+        uint256 approvedAt; // the time the transaction reached REQUIRED_CONFIRMATIONS (0 = not yet)
         bool executed;
     }

+    event TransactionApproved(uint256 indexed transactionId, uint256 approvedAt);

     // in _confirmTransaction, after the signature is stored
+        Transaction storage txn = s_transactions[txnId];
+        if (txn.approvedAt == 0 && _liveConfirmations(txnId) >= REQUIRED_CONFIRMATIONS) {
+            txn.approvedAt = block.timestamp;
+            emit TransactionApproved(txnId, block.timestamp);
+        }

     // in _revokeConfirmation, after the signature is removed
+        if (_liveConfirmations(txnId) < REQUIRED_CONFIRMATIONS) {
+            s_transactions[txnId].approvedAt = 0;
+        }

     // in _executeTransaction
+        if (txn.approvedAt == 0) {
+            revert MultiSigTimelock__TransactionNotApproved();
+        }
-        uint256 executionTime = txn.proposedAt + requiredDelay;
+        uint256 executionTime = txn.approvedAt + requiredDelay;

[M-3] A signer can call `renounceRole` and leave the signer list out of sync

Description:

AccessControl exposes a public renounceRole. A signer that calls it loses the role, but s_signers, s_isSigner and s_signerCount are not updated. The wallet now thinks the slot is used. The owner cannot add the account back with grantSigningRole because s_isSigner is still true, so it must first call revokeSigningRole to clean up. If enough signers do this the wallet can drop under three real signers while still reporting five.

Impact:

Likelihood Low

Risk Medium

Proof of Concept:

[PASS] test_M3_RenounceRoleDesyncsState()
Logs:
  signerCount    : 2
  hasRole(alice) : false

Recommended Mitigation:

Block renounceRole. A signer that wants out should be removed through revokeSigningRole so the list stays in sync. Also drop s_isSigner and use hasRole as the one source of truth (see L-5).

+    /// @dev Signers cannot leave on their own. Use revokeSigningRole through the wallet instead.
+    function renounceRole(bytes32, address) public pure override {
+        revert MultiSigTimelock__RenounceNotAllowed();
+    }

[M-4] Proposed transactions never expire and cannot be cancelled

Description:

There is no cancelTransaction and no deadline. A transaction proposed months ago, with confirmations still on it, can be executed whenever someone with the role decides to. Together with M-1 and M-2 this means an old forgotten proposal is a live risk forever. The only way to "cancel" is for signers to revoke their confirmations one by one.

Impact:

Likelihood Medium

Risk Medium

Proof of Concept:

N/A

Recommended Mitigation:

Reject execution once the approval is older than a fixed window, and add a cancelTransaction. Here the owner or the wallet itself can cancel, and cancelled transactions cannot be confirmed or executed any more.

+    error MultiSigTimelock__TransactionExpired(uint256 expiredAt);
+    /// @dev Constant for how long an approved transaction stays valid
+    uint256 private constant EXPIRY_PERIOD = 30 days;

     struct Transaction {
         ...
         bool executed;
+        bool cancelled;
     }

+    event TransactionCancelled(uint256 indexed transactionId);

+    modifier notCancelled(uint256 _transactionId) {
+        if (s_transactions[_transactionId].cancelled) {
+            revert MultiSigTimelock__TransactionDoesNotExist(_transactionId);
+        }
+        _;
+    }

+    function cancelTransaction(uint256 txnId)
+        external
+        transactionExists(txnId)
+        notExecuted(txnId)
+        notCancelled(txnId)
+    {
+        if (msg.sender != owner() && msg.sender != address(this)) revert MultiSigTimelock__OnlyWallet();
+        s_transactions[txnId].cancelled = true;
+        emit TransactionCancelled(txnId);
+    }

     // add notCancelled(txnId) to confirmTransaction, revokeConfirmation and executeTransaction

     // in _executeTransaction, right after the timelock check
+        if (block.timestamp > executionTime + EXPIRY_PERIOD) {
+            revert MultiSigTimelock__TransactionExpired(executionTime + EXPIRY_PERIOD);
+        }

Low

[L-1] Comment says the first signer cannot be removed, but the code only protects the last one

Description:

// src/MultiSigTimelock.sol:214-217
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner
if (s_signerCount <= 1) {
    revert MultiSigTimelock__CannotRevokeLastSigner();
}

The check only stops the very last signer from leaving. The owner can remove itself as a signer as long as another one exists. Not dangerous on its own, but the code does not match the intent written above it.

Impact:

Likelihood Low

Risk Low

Proof of Concept:

N/A

Recommended Mitigation:

With the M-1 fix the check becomes "never drop under the threshold", which is what the wallet really needs. Update the comment to match.

-        // Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
-        if (s_signerCount <= 1) {
+        // Never drop below the number of signers needed to execute, or the wallet is stuck
+        if (s_signerCount <= REQUIRED_CONFIRMATIONS) {
             revert MultiSigTimelock__CannotRevokeLastSigner();
         }

[L-2] Ownership transfer is one step

Description:

The contract uses Ownable, so transferOwnership hands control over immediately. A typo in the new address locks out the only account that can propose and manage signers, and the funds are stuck (see H-1).

Impact:

Likelihood Low

Risk Low

Proof of Concept:

N/A

Recommended Mitigation:

Use Ownable2Step so the new owner has to accept before the change takes effect.

-import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
+import {Ownable2Step, Ownable} from "@openzeppelin/contracts/access/Ownable2Step.sol";

-contract MultiSigTimelock is Ownable, AccessControl, ReentrancyGuard {
+contract MultiSigTimelock is Ownable2Step, AccessControl, ReentrancyGuard {

[L-3] Floating pragma

Description:

pragma solidity ^0.8.19; lets the contract be built with any 0.8.x compiler. Slither lists known bugs for that range, and Aderyn warns that 0.8.20 and above emit PUSH0, which some chains do not support yet.

Impact:

Likelihood Low

Risk Low

Proof of Concept:

N/A

Recommended Mitigation:

Pin one version and set the same in foundry.toml.

-pragma solidity ^0.8.19;
+pragma solidity 0.8.25;

 [profile.default]
 src = "src"
+solc = "0.8.25"

[L-4] Return value of `_grantRole` and `_revokeRole` is ignored

Description:

Aderyn flagged the three calls at lines 168, 199 and 239. OpenZeppelin returns bool to say if anything changed. The contract's own checks already make sure the role state is right, so this is safe today, but it will hide a bug if those checks ever change.

Impact:

Likelihood Low

Risk Low

Proof of Concept:

N/A

Recommended Mitigation:

The role must always change when these run, so treat a false return as a bug.

-        _grantRole(SIGNING_ROLE, _account);
+        assert(_grantRole(SIGNING_ROLE, _account));

-        _revokeRole(SIGNING_ROLE, _account);
+        assert(_revokeRole(SIGNING_ROLE, _account));

[L-5] Two sources of truth for who is a signer

Description:

s_isSigner and hasRole(SIGNING_ROLE, ...) are meant to be the same thing but live in two places. grantSigningRole checks the mapping while confirmTransaction checks the role. M-3 shows how they drift apart.

Impact:

Likelihood Low

Risk Low

Proof of Concept:

N/A

Recommended Mitigation:

Keep one. Remove s_isSigner and use hasRole everywhere.

-    /// @dev A mapping to quickly check if an address is an approved signer
-    mapping(address user => bool signer) private s_isSigner;

     // constructor
         s_signers[0] = msg.sender;
-        s_isSigner[msg.sender] = true;
         s_signerCount = 1;

     // grantSigningRole
-        if (s_isSigner[_account]) {
+        if (hasRole(SIGNING_ROLE, _account)) {
             revert MultiSigTimelock__AccountIsAlreadyASigner();
         }
         ...
         s_signers[s_signerCount] = _account;
-        s_isSigner[_account] = true;
         s_signerCount += 1;

     // revokeSigningRole
-        if (!s_isSigner[_account]) {
+        if (!hasRole(SIGNING_ROLE, _account)) {
             revert MultiSigTimelock__AccountIsNotASigner();
         }
         ...
-        s_isSigner[_account] = false;
         _revokeRole(SIGNING_ROLE, _account);

MultiSigTimelock Security Review

Table of contents

About Me

Disclaimer

Risk Classification

Audit Details

Scope

Protocol Summary

Roles

Executive Summary

Issues found

Findings

High

[H-1] The owner alone can control the "3 of 5" multisig

[H-2] The timelock only looks at ETH value, so token transfers and any contract call skip it

Medium

[M-1] A revoked signer's confirmation still counts

[M-2] The timelock starts at proposal time, not when the transaction is approved

[M-3] A signer can call renounceRole and leave the signer list out of sync

[M-4] Proposed transactions never expire and cannot be cancelled

Low

[L-1] Comment says the first signer cannot be removed, but the code only protects the last one

[L-2] Ownership transfer is one step

[L-3] Floating pragma

[L-4] Return value of _grantRole and _revokeRole is ignored

[L-5] Two sources of truth for who is a signer

[M-3] A signer can call `renounceRole` and leave the signer list out of sync

[L-4] Return value of `_grantRole` and `_revokeRole` is ignored