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

Revoked signer's confirmation remains valid, leading to a logic flaw and governance risk.

Author Revealed upon completion

Revoked signer's confirmation remains valid, leading to a logic flaw and governance risk.

Description

  • In the MultiSigTimelock contract, when a signer confirms a transaction, their confirmation is stored in the transaction struct. However, if the signer is later revoked via revokeSigningRole, their previous confirmation still counts toward the transaction’s confirmations. This means a signer who is no longer authorized can still contribute to the execution of already proposed transactions.

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) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
// 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);
}

Risk

  • Transactions can be executed with confirmations from revoked signers.

  • The multisig governance mechanism can be partially bypassed, potentially compromising the contract’s funds.


  • In this scenarios with a low minimum confirmation threshold (3/5), a revoked signer may enable execution of an unauthorized or malicious transaction.

Likelihood: Medium

  • This occurs during the process of revoking a signer who has already confirmed a pending transaction.

  • This also happens whenever signer roles are rotated or updated, which is a common operation in multisig management, making the scenario likely to be encountered.

Impact: Medium

  • A revoked signer’s previous confirmations remain counted, allowing a transaction to be executed even though that signer no longer has authority, which can lead to unauthorized or unexpected execution of transactions, breaking the intended access control and timelock guarantees.

  • It undermines the security assumptions of the multisig, potentially exposing funds to risk if the revoked signer was malicious or compromised.

Proof of Concept

Put the following code into the ".../test/uint/MultiSigTimelock.t.sol file :

Then, run : forge test --mt test_signature_still_counts_after_revoke_role -vv

function test_signature_still_counts_after_revoke_role() public {
address signer_1 = makeAddr("signer_1");
address signer_2 = makeAddr("signer_2");
address signer_3 = makeAddr("signer_3");
​
vm.prank(OWNER);
multiSigTimelock.grantSigningRole(signer_1);
​
vm.prank(OWNER);
multiSigTimelock.grantSigningRole(signer_2);
​
vm.prank(OWNER);
multiSigTimelock.grantSigningRole(signer_3);
​
vm.prank(OWNER);
uint256 txId = multiSigTimelock.proposeTransaction(address(0xdead), 1 ether, hex"");
​
//The signer_1 confirms;
​
vm.prank(signer_1);
multiSigTimelock.confirmTransaction(txId);
​
// Verification
MultiSigTimelock.Transaction memory confirmationBefore = multiSigTimelock.getTransaction(txId);
uint256 confirmationBeforeCount = confirmationBefore.confirmations;
assertEq(confirmationBeforeCount, 1);
console2.log("Confirmations before revok:, confirmationBeforeCount);
​
vm.prank(OWNER);
multiSigTimelock.revokeSigningRole(signer_1);
​
assertFalse(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), signer_1));
​
MultiSigTimelock.Transaction memory confirmationAfter = multiSigTimelock.getTransaction(txId);
uint256 confirmationsAfterCount = confirmationAfter.confirmations;
console2.log("Confirmations after revoke:", confirmationsAfterCount);
​
assertEq(confirmationsAfterCount, 1, "Confirmation count should remain the same after revoking signing role");
}

Recommended Mitigation

  • Instead of storing the confirmations count in the transaction, calculate it dynamically by counting only active signers at the moment of execution.

  • Or follow the code mitigation below :

// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
+ for (uint256 i = 0; i < s_transactionCount; i++); {
+ if (s_transaction[i].executed) continue;
+ if (s_signatures[i][_account]) {
+ s_signatures[i][_account] = false;
+ s_transactions[i].confirmations--;
+ }
+ }
​
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!