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

19Dec2025_AuditReport6_

Author Revealed upon completion

Root + Impact

Description

  • Unbounded Loop in revokeSigningRole Can Cause DoS

  • The revokeSigningRole function contains an unbounded loop that iterates through all signers to find the target address. While currently limited to 5 signers, if the MAX_SIGNER_COUNT were increased or if the loop fails to find the signer due to a bug, this could consume excessive gas. Additionally, the pattern of using type(uint256).max as a sentinel value without checking if the signer was actually found could lead to unexpected behavior.

// Root cause in the codebase with @> marks to highlight the relevant section
// If s_isSigner[account] = true but account is not in s_signers array
// (possible due to state inconsistency bug)
multiSigTimelock.revokeSigningRole(accountNotInArray);
// indexToRemove remains type(uint256).max
// The comparison indexToRemove < s_signerCount - 1 will be false
// But the function continues to execute, potentially in an invalid state

Risk

Likelihood:

  • The revokeSigningRole function contains an unbounded loop that iterates through all signers to find the target address. While currently limited to 5 signers, if the MAX_SIGNER_COUNT were increased or if the loop fails to find the signer due to a bug, this could consume excessive gas. Additionally, the pattern of using type(uint256).max as a sentinel value without checking if the signer was actually found could lead to unexpected behavior.

Impact:

  • If the signer is not found in the array (due to state inconsistency), the function will proceed with indexToRemove = type(uint256).max, potentially causing an out-of-bounds array access or unexpected behavior. Gas costs increase linearly with signer count.

Proof of Concept

The "Swap and Pop" Logic

The code uses a gas-efficient method to remove an address from the s_signers array. Instead of shifting every item, it finds the target's position and replaces it with the last item in the list.

Key Components:

• The Search: It loops through the array to find the index of the address provided.

• The Swap: It overwrites the target address with the address currently sitting at the end of the array.

• The Flaw: If the address isn't found, indexToRemove stays at the maximum possible value. Because there is no "if not found, revert" check, the function might proceed to delete the last signer in the list by mistake.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// ...
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;
}
}
// Logic to swap the element to remove with the last element
if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
// ...
}
​

Recommended Mitigation

By adding the require statement, you ensure the function only proceeds if the address is actually in the list.

Why this matters:

• Prevents Wrongful Deletion: Without this check, the function would skip the "swap" logic but still decrease the signer count, accidentally removing the last person in the list.

• State Integrity: It ensures your s_signerCount stays accurate and synced with the actual addresses in storage.

• Clear Feedback: It provides an explicit error message ("Signer not found") if an admin tries to remove someone who isn't a signer.

//Add explicit validation that the signer was found:
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// ...
uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
// Explicit validation: ensures the loop actually found the address
require(indexToRemove != type(uint256).max, "Signer not found in array");
// Swap the target with the last element
if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// Reduce the count and clean up the storage
s_signerCount--;
delete s_signers[s_signerCount];
}
​

Support

FAQs

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

Give us feedback!