revokeSigningRole() does not check if _account is the owner + the owner's signer role can be revoked allowing 5 other signers
Description
-
revokeSigningRole() should check if _account is the owner to keep the "owner + up to 4 others that possess the SIGNING_ROLE" invariant.
-
Tthe owner's signer role can be revoked allowing 5 other signers.
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;
}
}
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:
Impact:
Proof of Concept
Lines 1 - 3: The owner and 4 other usrs are granted the signer role. This total 5 uers with the role.
Lines 5 - 7: The owner is revoked of their signer role.
Lines 9 - 11: Another user is granted the signer role
Lines 12 - 13: We get the signerCount again confirming that there are now 5 users with the signer role and non of them is the admin.
function testCanHaveFiveOtherSigners() public grantSigningRoles {
uint256 signerCount = multiSigTimelock.getSignerCount();
assertEq(signerCount, 5);
multiSigTimelock.revokeSigningRole(OWNER);
signerCount = multiSigTimelock.getSignerCount();
assertEq(signerCount, 4);
address SIGNER_SIX = makeAddr("signer_six");
multiSigTimelock.grantSigningRole(SIGNER_SIX);
signerCount = multiSigTimelock.getSignerCount();
assertEq(signerCount, 5);
}
Run with:
forge test -vvv --match-test testCanHaveFiveOtherSigners
Sample output:
Ran 1 test for test/unit/MultiSigTimelockTest.t.sol:MultiSigTimeLockTest
[PASS] testCanHaveFiveOtherSigners() (gas: 386846)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.88ms (1.65ms CPU time)
Ran 1 test suite in 25.34ms (8.88ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
+
+ if (_account == owner()) {
+ revert("can't revoke owner");
+ }
+
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
// Prevent revoking the first signer (would break the multisig), moreover, the first signer is the owner of the contract(wallet)
if (s_signerCount <= 1) {
revert MultiSigTimelock__CannotRevokeLastSigner();
}
// 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;
}
}
// Gas-efficient array removal: move last element to removed position
if (indexToRemove < s_signerCount - 1) {
// Move the last signer to the position of the removed signer
s_signers[indexToRemove] = s_signers[s_signerCount - 1];
}
// Clear the last position and decrement count
s_signers[s_signerCount - 1] = address(0);
s_signerCount -= 1;
s_isSigner[_account] = false;
_revokeRole(SIGNING_ROLE, _account);
}
