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

A just revoked signing role may have already signed a not-executed transaction causing a transaction to be executed without approval of enough current signers

Author Revealed upon completion

Root + Impact

Description

  • When the owner revokes a signer's role in the MultiSigTimelock::revokeSigningRole function, the user may have already signed a proposed transaction. The issue is it will be counted towards the required threshold of MultiSigTimelock::REQUIRED_CONFIRMATIONS constant.

  • This way, if only 2 of the current signers confirm the transaction, it will be passed and ready for execution while it should not be.

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

  • It is quite likely in the process of a live protocol to have many open (not yet executed) proposed transactions.

Impact:

  • The impact can be severe due to the confirmation made by someone who does not have any responsibility about the consequences of their decisions anymore.

Proof of Concept

Please add the following function to the test file and run it using forge test --mt testRevokedSignerCanAffectConfirmations -vvvv command in the terminal window.

function testRevokedSignerCanAffectConfirmations() public grantSigningRoles {
// Arrange
vm.deal(address(multiSigTimelock), OWNER_BALANCE_TWO);
​
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
​
// Act
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
​
vm.prank(OWNER);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_FOUR);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId);
// Assert
assertTrue(multiSigTimelock.getTransaction(txnId).executed, "The transaction failed to execute due to not enough signers' confirmation");
}

Recommended Mitigation

To solve the issue make the following changes. However, due to the huge number of transactions over time it can cause DoS and run out of gas. So the proper solution is to have an array of the current (not yet executed) transactions and traverse it looking for the ones signed by the already revoked signer.

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;
​
+ for (uint256 i = 0; i < s_transactionCount; i++) {
+ if (!s_transactions[i].executed && s_signatures[i][_account]) {
+ s_transactions[i].confirmations--;
+ }
+ }
_revokeRole(SIGNING_ROLE, _account);
}
​

Support

FAQs

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

Give us feedback!