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

Missing Existence Check for indexToRemove in 'multiSigTimeLock::revokeSigningRole'

Author Revealed upon completion

Root + Impact

if the s_isSigner mapping and the s_signers array were ever to become inconsistent, the removal logic could:

  • Attempt to operate on an invalid array index

  • Corrupt the s_signers array

  • Leave signer state in an inconsistent or undefined condition

Description

The 'revokeSigningRole' function removes a signer by first checking the signer's index in the s_signers array before actually removing the signer. While the function checks s_isSigner[_account] before proceeding, it does not explicitly verify that the signer's index was successfully found in the array.

If the index is not found, indexToRemove remains set to type(uint256).max, and the function continues execution without reverting.

// Root cause in the codebase with @> marks // 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;
}
}to highlight the relevant section

Risk

Likelihood:

  • Lets say a previous function or transaction incorrectly modifies the contract's state, it is possible that the s_isSigner mapping marks an address as a signer, but the address is not added or removed from the s_signers array


Impact:

  • Attempt to operate on an invalid array index

  • corrup the s_signers array

Proof of Concept

In a scenario where the contract state is corrupted such that s_isSigner[_account] == true but _account is not present in the s_signers array, the loop that searches for the signer would fail to find a matching index. As a result, indexToRemove would remain set to type(uint256).max, and the subsequent array removal logic could attempt to overwrite an invalid array position, potentially corrupting contract state. While this situation cannot occur during normal operation, adding the recommended check ensures the function fails safely even in unexpected or corrupted states.

Recommended Mitigation

+ if (indexToRemove == type(uint256).max) {
+ revert MultiSigTimelock__SignerIndexNotFound();
}

Support

FAQs

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

Give us feedback!