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

Gas Inefficiency in Signer Confirmation Validation

Author Revealed upon completion

Description

  • The contract stores a confirmation counter in the Transaction struct and separately tracks individual signer confirmations in the s_signatures nested mapping.

  • During execution, the contract only checks the counter txn.confirmations >= REQUIRED_CONFIRMATIONS without validating that these confirmations are from current valid signers. This is gas-inefficient for verification and can lead to state inconsistencies.

struct Transaction {
address to;
uint256 value;
bytes data;
@> uint256 confirmations; // Counter that can become stale
uint256 proposedAt;
bool executed;
}
​
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
@> // Only checks counter, doesn't validate signers are still valid
@> if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
// ... rest of execution
}

Risk

Likelihood:

  • When executing any transaction, the contract performs a simple counter check that may not reflect the current valid signer set

  • This redundant storage (counter + mapping) increases gas costs for confirmations and revocations

Impact:

  • Slightly higher gas costs for all confirmation and revocation operations

  • Risk of counter becoming out-of-sync with actual valid signatures (as shown in MEDIUM finding above)

  • Unnecessary storage slot usage

Proof of Concept

function testRedundantConfirmationCounter() public {
uint256 txId = multiSig.proposeTransaction(recipient, 1 ether, "");
// Confirmation updates both mapping AND counter
multiSig.confirmTransaction(txId);
// Storage writes:
// 1. s_signatures[txId][msg.sender] = true (1 SSTORE)
// 2. s_transactions[txId].confirmations++ (1 SSTORE)
// Total: 2 SSTOREs when we could use just the mapping
}

Recommended Mitigation

struct Transaction {
address to;
uint256 value;
bytes data;
- uint256 confirmations; // Remove redundant counter
uint256 proposedAt;
bool executed;
}
​
function _confirmTransaction(uint256 txnId) internal {
if (s_signatures[txnId][msg.sender]) {
revert MultiSigTimeLock__UserAlreadySigned();
}
s_signatures[txnId][msg.sender] = true;
​
- // Remove counter increment
- 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;
​
- // Remove counter decrement
- s_transactions[txnId].confirmations--;
​
emit TransactionRevoked(txnId, msg.sender);
}
​
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
​
// CHECKS
- // Check counter
- if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
- revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
- }
​
+ // Count valid confirmations from current signers
+ uint256 validConfirmations = _countValidConfirmations(txnId);
+ if (validConfirmations < REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, validConfirmations);
+ }
​
// ... rest of function
}
​
+ function _countValidConfirmations(uint256 txnId) internal view returns (uint256) {
+ uint256 count = 0;
+ for (uint256 i = 0; i < s_signerCount; i++) {
+ if (s_signers[i] != address(0) && s_signatures[txnId][s_signers[i]]) {
+ count++;
+ }
+ }
+ return count;
+ }

Support

FAQs

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

Give us feedback!