Description
-
The contract increments the confirmation counter when signers confirm transactions and decrements it when they revoke their confirmation.
-
In the edge case where a signer confirms a transaction, then later revokes their confirmation, and the owner then revokes their signing role entirely, the confirmation counter becomes out of sync. If the signer's confirmation is revoked AFTER their role is revoked (by replaying the revoke confirmation call), an underflow could theoretically occur, though Solidity 0.8+ prevents this with a revert.
function _confirmTransaction(uint256 txnId) internal {
if (s_signatures[txnId][msg.sender]) {
revert MultiSigTimeLock__UserAlreadySigned();
}
s_signatures[txnId][msg.sender] = true;
@>
@> s_transactions[txnId].confirmations++;
emit TransactionConfirmed(txnId, msg.sender);
}
function _revokeConfirmation(uint256 txnId) internal {
if (!s_signatures[txnId][msg.sender]) {
revert MultiSigTimeLock__UserHasNotSigned();
}
s_signatures[txnId][msg.sender] = false;
@>
@> s_transactions[txnId].confirmations--;
emit TransactionRevoked(txnId, msg.sender);
}
Risk
Likelihood:
-
When a signer confirms a transaction, their role gets revoked, but they can still call revokeConfirmation due to the onlyRole modifier check happening after the signature check
-
This creates a mismatch between the confirmation counter and actual valid confirmations
Impact:
-
Confirmation count becomes inaccurate and doesn't reflect actual valid signatures
-
Could lead to transaction execution with fewer actual valid signers than required
-
State inconsistency between confirmation count and signature mapping
Proof of Concept
function testConfirmationCountMismatch() public {
multiSig.grantSigningRole(signer1);
uint256 txId = multiSig.proposeTransaction(recipient, 1 ether, "");
vm.prank(signer1);
multiSig.confirmTransaction(txId);
assertEq(multiSig.getTransaction(txId).confirmations, 1);
multiSig.revokeSigningRole(signer1);
assertEq(multiSig.getTransaction(txId).confirmations, 1);
}
Recommended Mitigation
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
// CHECKS
- // 1. Check if enough confirmations
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
- revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
- }
+ // 1. Check if enough VALID confirmations (from current signers only)
+ uint256 validConfirmations = _countValidConfirmations(txnId);
+ if (validConfirmations < REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, validConfirmations);
+ }
// 2. Check if timelock period has passed
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
// ... rest of function
}
+ function _countValidConfirmations(uint256 txnId) internal view returns (uint256) {
+ uint256 count = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ address signer = s_signers[i];
+ if (signer != address(0) && s_signatures[txnId][signer]) {
+ count++;
+ }
+ }
+ return count;
+ }
