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

revokeSigningRole Can Corrupt the Signer Set Because indexToRemove Is Not Validated

Author Revealed upon completion

Root + Impact

Description

  • Normal behavior: When the owner revokes a signer, the contract should locate the signer in s_signers[0..s_signerCount-1] and remove exactly that entry using swap-and-pop, keeping s_signers, s_signerCount, s_isSigner, and AccessControl membership consistent.

  • Issue: revokeSigningRole assumes that any account with s_isSigner[_account] == true must also exist in the s_signers array, but it never validates that the search found the index. If the signer is not found, indexToRemove stays as type(uint256).max, and the function silently clears the last signer slot and decrements s_signerCount, while revoking the role for _account. This desynchronizes signer enumeration and role membership, potentially bricking signer administration.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
​
// @> uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// @> if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// @> s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Risk

Likelihood:

  • The function initializes indexToRemove as a sentinel value. It then searches for the signer in the array. However, if the signer is not found, no check is performed before continuing execution. The function then blindly executes swap-and-pop logic using the invalid index. Because indexToRemove == type(uint256).max, the condition is false and the function removes the last signer in the array, not the intended one.

Impact:

  • Removes/clears the wrong signer entry (the last active slot), corrupting the enumerated signer set and s_signerCount.


Proof of Concept

Paste the following test into MultiSigTimelock.t.sol.

This PoC forces a realistic invariant break (custom signer mapping says “signer”, but signer array does not include them) by writing to storage, then demonstrates that revokeSigningRole silently removes the wrong signer.

function testPoC_H01_RevokeSigningRoleMissingIndexValidationCorruptsSignerSet() public {
// -------------------------
// 1) Setup: reach 5 signers
// -------------------------
multiSigTimelock.grantSigningRole(SIGNER_TWO);
multiSigTimelock.grantSigningRole(SIGNER_THREE);
multiSigTimelock.grantSigningRole(SIGNER_FOUR);
multiSigTimelock.grantSigningRole(SIGNER_FIVE);
​
assertEq(multiSigTimelock.getSignerCount(), 5);
assertTrue(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_FIVE));
​
address phantom = makeAddr("phantom_signer");
​
// -------------------------------------------------------------------
// 2) Discover the storage slot 'p' of mapping(address=>bool) s_isSigner
// by observing where grantSigningRole writes for a known key.
// -------------------------------------------------------------------
uint256 p = _findIsSignerMappingSlot();
​
// mapping slot for s_isSigner[phantom] is keccak256(abi.encode(phantom, p))
bytes32 phantomSlot = keccak256(abi.encode(phantom, p));
​
// Force: s_isSigner[phantom] = true (but phantom is NOT in s_signers array)
vm.store(address(multiSigTimelock), phantomSlot, bytes32(uint256(1)));
​
// Sanity: phantom still does NOT have the AccessControl role
assertFalse(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), phantom));
​
// -------------------------------------------------------------------
// 3) Trigger the bug: revokeSigningRole(phantom) passes s_isSigner check,
// but phantom isn't found in s_signers, so indexToRemove stays max
// and the function clears the last signer slot instead.
// -------------------------------------------------------------------
multiSigTimelock.revokeSigningRole(phantom);
​
// The signer count drops even though phantom wasn't actually present in s_signers
assertEq(multiSigTimelock.getSignerCount(), 4);
​
// SIGNER_FIVE still has the role, but may no longer be present in the enumerated signer list
assertTrue(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_FIVE));
​
address[5] memory signers = multiSigTimelock.getSigners();
​
// Since SIGNER_FIVE was last, the bug path clears signers[4] and decrements count
assertEq(signers[4], address(0));
​
// SIGNER_FIVE should not appear in the active range [0..3] anymore
bool foundInActiveRange = false;
for (uint256 i = 0; i < 4; i++) {
if (signers[i] == SIGNER_FIVE) {
foundInActiveRange = true;
break;
}
}
assertFalse(foundInActiveRange);
}
​
// Helper: find the mapping slot index 'p' for s_isSigner by recording writes.
function _findIsSignerMappingSlot() internal returns (uint256) {
// Deploy a fresh instance to observe a clean grantSigningRole write pattern
MultiSigTimelock fresh = new MultiSigTimelock();
address key = makeAddr("slot_probe_signer");
​
vm.record();
fresh.grantSigningRole(key);
(, bytes32[] memory writes) = vm.accesses(address(fresh));
​
// Brute force small slot indices; inherited storage pushes our vars forward,
// so we search a reasonable range.
for (uint256 candidate = 0; candidate < 300; candidate++) {
bytes32 computed = keccak256(abi.encode(key, candidate));
for (uint256 j = 0; j < writes.length; j++) {
if (writes[j] == computed) {
return candidate;
}
}
}
​
revert("mapping slot not found");
}

Recommended Mitigation

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
​
uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
+ // Ensure the signer actually exists in the enumerated signer set
+ if (indexToRemove == type(uint256).max) {
+ revert MultiSigTimelock__AccountIsNotASigner();
+ }
​
if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}

Support

FAQs

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

Give us feedback!