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

Silent unintended signer removal when array/mapping desync occurs

Author Revealed upon completion

Description

  • When revoking a signer, the contract should locate the signer’s index in s_signers and remove that specific address, keeping the array/mapping consistent. If the signer is not present in the array, the operation must revert.

  • revokeSigningRole uses indexToRemove = type(uint256).max as a sentinel during the search loop but never checks that the index was found. If s_isSigner[_account] == true while _account is not present in s_signers (array/mapping desynchronization), the function skips the swap and still “pops” the last element, clearing a different signer and decrementing s_signerCount. This produces a silent, wrong removal.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
​
// Find the index of the account in the array
uint256 indexToRemove = type(uint256).max; // sentinel
for (uint256 i = 0; i < s_signerCount; i++) {
if (s_signers[i] == _account) {
indexToRemove = i;
break;
}
}
​
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
​
// @> Missing: validation that indexToRemove was found.
// @> Always clears the last slot and decrements the count, even if 'indexToRemove' was never found.
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
​
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}
​

Risk

Likelihood: Low

  • Whenever state becomes inconsistent (e.g., due to prior bugs, manual storage edits in tests, migrations, or a future refactor that forgets to update both structures), calling revokeSigningRole on the “phantom” signer will silently remove the wrong (last) signer.

  • This can also occur in edge cases during incident response or tooling that directly manipulates storage.

Impact: Medium

  • ncorrect signer removal & governance disruption: A valid signer is removed unexpectedly, reducing s_signerCount and potentially dropping the set below quorum (see prior deadlock bug).

  • Stealth failure: No revert or event indicates the wrong target was removed, making forensics and recovery harder.

Proof of Concept

The inconsistent state (mapping says true, array lacks the address) cannot be reached via the public API alone, but it can be reproduced with a Foundry cheatcode (vm.store) or any migration script that accidentally desynchronizes storage. Once desynchronized, calling revokeSigningRole(phantomSigner) will “pop” the last signer rather than the target, without reverting.

  • Create poc1.t.sol under unit directory and copy the code below.

  • Run forge test --mp poc1 -vv.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {Test, console2} from "forge-std/Test.sol";
import {MultiSigTimelock} from "src/MultiSigTimelock.sol";
​
​
/**
* PoC: "Swap-and-pop without index validation removes unintended signer".
*
* Strategy:
* - Add four signers so the fixed array has: [owner, S2, S3, PHANTOM, S4].
* - Find the base storage slot of s_signers[] by scanning for the contiguous pattern.
* - Zero-out only the PHANTOM array slot (keep mapping/role TRUE via grantSigningRole).
* - Call revokeSigningRole(PHANTOM); since PHANTOM is not in the array, the code
* will clear the LAST slot and decrement count — removing S4 instead (wrong signer).
*/
contract POC1_SilentWrongRemoval is Test {
MultiSigTimelock private ms;
​
// actors
address private OWNER = address(this); // deployer is owner
address private SIGNER_TWO = address(0xB0B);
address private SIGNER_THREE = address(0xC0C);
address private PHANTOM = address(0xD00D); // will be desynced from array
address private SIGNER_FOUR = address(0xE0E);
​
function setUp() public {
ms = new MultiSigTimelock();
​
// Owner is the initial signer
assertEq(ms.getSignerCount(), 1);
​
// Add S2, S3, PHANTOM, S4 => 5 signers total (max)
ms.grantSigningRole(SIGNER_TWO);
ms.grantSigningRole(SIGNER_THREE);
ms.grantSigningRole(PHANTOM); // PHANTOM now in array AND mapping
ms.grantSigningRole(SIGNER_FOUR);
​
assertEq(ms.getSignerCount(), 5, "expect max 5 signers");
​
address[5] memory signers = ms.getSigners();
assertEq(signers[0], OWNER);
assertEq(signers[1], SIGNER_TWO);
assertEq(signers[2], SIGNER_THREE);
assertEq(signers[3], PHANTOM); // we will zero-out this slot
assertEq(signers[4], SIGNER_FOUR);
}
​
/**
* Find the base storage slot of the fixed array s_signers[5] by scanning for
* a contiguous pattern of the five known addresses across storage slots.
* This avoids relying on inheritance layout / base contract slots.
*/
function _findSignersBaseSlot() internal view returns (uint256 base) {
// We search a reasonable range of slots for the 5-consecutive addresses.
for (uint256 s = 0; s < 2048; s++) {
if (_slotHasAddress(s, OWNER) &&
_slotHasAddress(s + 1, SIGNER_TWO) &&
_slotHasAddress(s + 2, SIGNER_THREE) &&
_slotHasAddress(s + 3, PHANTOM) &&
_slotHasAddress(s + 4, SIGNER_FOUR))
{
return s; // found the base
}
}
revert("signers base slot not found");
}
​
/// Read a storage slot and check if it holds `addr` (address is right-truncated in 32 bytes).
function _slotHasAddress(uint256 slot, address addr) internal view returns (bool) {
bytes32 raw = vm.load(address(ms), bytes32(slot));
// address is stored in the lower 20 bytes of the 32-byte word
address decoded = address(uint160(uint256(raw)));
return decoded == addr;
}
​
function test_poc1_SilentWrongRemoval() public {
console2.log("Pre signerCount:", ms.getSignerCount());
​
// 1) Locate the base slot of s_signers[]
uint256 base = _findSignersBaseSlot();
console2.log("Found s_signers base slot:", base);
​
// 2) Zero-out ONLY the PHANTOM slot (index 3) to desync array while mapping/role stay true
// Array becomes: [owner, S2, S3, 0x0, S4]
vm.store(address(ms), bytes32(base + 3), bytes32(uint256(0)));
​
// Sanity: read back the array via getter
{
address[5] memory signers = ms.getSigners();
assertEq(signers[0], OWNER, "index0 owner");
assertEq(signers[1], SIGNER_TWO, "index1 S2");
assertEq(signers[2], SIGNER_THREE, "index2 S3");
assertEq(signers[3], address(0), "index3 zeroed PHANTOM");
assertEq(signers[4], SIGNER_FOUR, "index4 S4");
}
​
// Mapping and role for PHANTOM are still true (we granted it)
bytes32 SIGNING_ROLE = ms.getSigningRole();
assertTrue(ms.hasRole(SIGNING_ROLE, PHANTOM), "PHANTOM still has role (mapping true)");
​
// 3) Call revokeSigningRole on PHANTOM: index search fails, function clears LAST slot (S4)
ms.revokeSigningRole(PHANTOM);
​
// 4) Assert: WRONG signer removed — last slot cleared and count decremented
uint256 afterCount = ms.getSignerCount();
console2.log("Post signerCount:", afterCount);
assertEq(afterCount, 4, "count decreased by 1");
​
address[5] memory afterSigners = ms.getSigners();
assertEq(afterSigners[0], OWNER, "owner intact");
assertEq(afterSigners[1], SIGNER_TWO, "S2 intact");
assertEq(afterSigners[2], SIGNER_THREE, "S3 intact");
​
// The last slot was cleared by revokeSigningRole(...) => S4 wrongly removed
assertEq(afterSigners[3], address(0), "index3 remains zero");
assertEq(afterSigners[4], address(0), "index4 cleared (popped last signer)");
​
// 5) Role mismatch evidence: S4 STILL HAS SIGNING_ROLE (because _revokeRole ran for PHANTOM)
assertTrue(ms.hasRole(SIGNING_ROLE, SIGNER_FOUR), "S4 still has role (state inconsistency)");
​
// PHANTOM lost the role (as revoke targeted PHANTOM), but the array never contained PHANTOM
assertFalse(ms.hasRole(SIGNING_ROLE, PHANTOM), "PHANTOM role revoked");
}
}

Output:

[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/unit/poc1.t.sol:POC1_SilentWrongRemoval
[PASS] test_poc1_SilentWrongRemoval() (gas: 90285)
Logs:
Pre signerCount: 5
Found s_signers base slot: 3
Post signerCount: 4
​
Traces:
[107485] POC1_SilentWrongRemoval::test_poc1_SilentWrongRemoval()
├─ [2543] MultiSigTimelock::getSignerCount() [staticcall]
│ └─ ← [Return] 5
├─ [0] console::log("Pre signerCount:", 5) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000001) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000000
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000001) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000000
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000002) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000005
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000003) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000004) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000b0b
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000005) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000c0c
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000006) [staticcall]
│ └─ ← [Return] 0x000000000000000000000000000000000000000000000000000000000000d00d
├─ [0] VM::load(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000007) [staticcall]
│ └─ ← [Return] 0x0000000000000000000000000000000000000000000000000000000000000e0e
├─ [0] console::log("Found s_signers base slot:", 3) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::store(MultiSigTimelock: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0x0000000000000000000000000000000000000000000000000000000000000006, 0x0000000000000000000000000000000000000000000000000000000000000000)
│ └─ ← [Return]
├─ [3290] MultiSigTimelock::getSigners() [staticcall]
│ └─ ← [Return] [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496, 0x0000000000000000000000000000000000000B0b, 0x0000000000000000000000000000000000000C0C, 0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000e0E]
├─ [0] VM::assertEq(POC1_SilentWrongRemoval: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], POC1_SilentWrongRemoval: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], "index0 owner") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000B0b, 0x0000000000000000000000000000000000000B0b, "index1 S2") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000C0C, 0x0000000000000000000000000000000000000C0C, "index2 S3") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000, "index3 zeroed PHANTOM") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000e0E, 0x0000000000000000000000000000000000000e0E, "index4 S4") [staticcall]
│ └─ ← [Return]
├─ [465] MultiSigTimelock::getSigningRole() [staticcall]
│ └─ ← [Return] 0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b
├─ [3166] MultiSigTimelock::hasRole(0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, 0x000000000000000000000000000000000000D00d) [staticcall]
│ └─ ← [Return] true
├─ [0] VM::assertTrue(true, "PHANTOM still has role (mapping true)") [staticcall]
│ └─ ← [Return]
├─ [26276] MultiSigTimelock::revokeSigningRole(0x000000000000000000000000000000000000D00d)
│ ├─ emit RoleRevoked(role: 0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, account: 0x000000000000000000000000000000000000D00d, sender: POC1_SilentWrongRemoval: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Stop]
├─ [543] MultiSigTimelock::getSignerCount() [staticcall]
│ └─ ← [Return] 4
├─ [0] console::log("Post signerCount:", 4) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::assertEq(4, 4, "count decreased by 1") [staticcall]
│ └─ ← [Return]
├─ [3290] MultiSigTimelock::getSigners() [staticcall]
│ └─ ← [Return] [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496, 0x0000000000000000000000000000000000000B0b, 0x0000000000000000000000000000000000000C0C, 0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000]
├─ [0] VM::assertEq(POC1_SilentWrongRemoval: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], POC1_SilentWrongRemoval: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], "owner intact") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000B0b, 0x0000000000000000000000000000000000000B0b, "S2 intact") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000C0C, 0x0000000000000000000000000000000000000C0C, "S3 intact") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000, "index3 remains zero") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(0x0000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000, "index4 cleared (popped last signer)") [staticcall]
│ └─ ← [Return]
├─ [3166] MultiSigTimelock::hasRole(0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, 0x0000000000000000000000000000000000000e0E) [staticcall]
│ └─ ← [Return] true
├─ [0] VM::assertTrue(true, "S4 still has role (state inconsistency)") [staticcall]
│ └─ ← [Return]
├─ [1166] MultiSigTimelock::hasRole(0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, 0x000000000000000000000000000000000000D00d) [staticcall]
│ └─ ← [Return] false
├─ [0] VM::assertFalse(false, "PHANTOM role revoked") [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.59ms (610.70µs CPU time)
​
Ran 1 test suite in 17.76ms (1.59ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • Validate that the index was found before modifying the array; revert otherwise.

+ error MultiSigTimelock__AccountNotFoundInArray(address account);
​
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;
}
}
​
+ // Require that the signer exists in the array to avoid popping the wrong element
+ if (indexToRemove == type(uint256).max) {
+ revert MultiSigTimelock__AccountNotFoundInArray(_account);
+ }
​
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!