Root + Impact
Description
-
A multisig transaction should only be executable if it has the required number of confirmations from currently authorized signers at the time of execution. If a signer is revoked before execution, any prior confirmations from that signer should be invalidated and must not be counted toward the execution threshold.
-
The contract allows a transaction to be executed using confirmations from a signer who has been revoked, because signer validity is not rechecked or prior confirmations are not invalidated at execution time
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
@> if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
if (txn.value > address(this).balance) {
revert MultiSigTimelock__InsufficientBalance(address(this).balance);
}
txn.executed = true;
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) {
revert MultiSigTimelock__ExecutionFailed();
}
emit TransactionExecuted(txnId, txn.to, txn.value);
}
--------------------------------------------------------------
struct Transaction {
address to;
uint256 value;
bytes data;
@> uint256 confirmations;
uint256 proposedAt;
bool executed;
}
function revokeSigningRole(address _account) external nonReentrant onlyOwner {
...
s_isSigner[_account] = false;
@> _revokeRole(SIGNING_ROLE, _account);
}
Risk
Likelihood:
This occurs when a signer confirms a transaction, the signer’s signing role is later revoked before execution, and the transaction is executed after the timelock using the previously recorded confirmation count, which still includes the revoked signer’s approval.
Impact:
Proof of Concept
Please add the following test function in testRevokedSignerConfirmationStillCounts() in MultiSigTimelockTest.t.sol and run forge test --match-test testRevokedSignerConfirmationStillCounts and the test will pass.
function testRevokedSignerConfirmationStillCounts() public grantSigningRoles {
vm.deal(address(multiSigTimelock), 5 ether);
vm.prank(OWNER);
uint256 txId = multiSigTimelock.proposeTransaction(SPENDER_ONE, 1 ether, "");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txId);
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
vm.warp(block.timestamp + 8 days);
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txId);
}
Recommended Mitigation
Mitigation can be added that when a signer is revoked, the contract iterates through all proposed transactions. It finds transactions that are not yet executed and were previously confirmed by the revoked signer. It removes the signer’s confirmation and decrements the confirmation counter accordingly
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();
}
@> for (uint256 i = 0; i < s_transactionCount; i++) {
@> if (!s_transactions[i].executed && s_signatures[i][_account]) {
@> s_signatures[i][_account] = false;
@> s_transactions[i].confirmations--;
@> }
@> }
// 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);
}
