Submission Details
Impact:
Likelihood:
Transaction can stuck if total number of signers goes below REQUIRED_CONFIRMATIONS
Root + Impact
Description
-
Protocol works with transactions being signed by at least 3 different signers (REQUIRED_CONFIRMATIONS) in order to execute proposed transactions.
-
Owner can remove signers until 1 left, this way transactions can be locked and never proceed.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
@> // No check for treshold
// 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);
}
Risk
Likelihood:
This can happen when owner decides to revoke every signer.
Impact:
Systems permamently stuck until owner adds new signers. No way to confirm pending transactions.
Proof of Concept
function testRevokeSigningRole() public grantSigningRoles {
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
multiSigTimelock.revokeSigningRole(SIGNER_THREE);
multiSigTimelock.revokeSigningRole(SIGNER_FOUR);
multiSigTimelock.revokeSigningRole(SIGNER_FIVE);
}
Recommended Mitigation
If check can be added to control if number of signers are going below treshold.
//-- WHERE ERRORS DECLARED --
+ error MultiSigTimelock__CannotRevokeBelowTreshold();
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// CHECKS
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();
- }
+ // Prevent revoking below REQUIRED_COFIRMATIOS to not lock the system.
+ if (s_signerCount <= REQUIRED_CONFIRMATIONS) {
+ revert MultiSigTimelock__CannotRevokeBelowTreshold();
+ }
// 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);
}
Rewards breakdown
nSLOC
205This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.