Root + Impact
Description
-
The revokeSigningRole first checks if an account is a signer and then checks if it is the last signer of a transaction preventing role revokal.
-
It then uses the swap and pop method for array removal from the list of signers for that specific transaction.
-
A signer is allowed for a transaction and also does the confirmation from his end, the list of confirmation sees an increment in value. If the deployer chooses to revoke the role from the signer, the number of confirmations is not changed.
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
function testConfirmationNotRemovedWhenRoleRevoked() public grantSigningRoles {
vm.deal(OWNER, OWNER_BALANCE_ONE);
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
assertEq(multiSigTimelock.getTransaction(txnId).confirmations, 3);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
assertEq(multiSigTimelock.getTransaction(txnId).confirmations, 3);
assertFalse(multiSigTimelock.hasRole(multiSigTimelock.getSigningRole(), SIGNER_TWO));
vm.prank(SIGNER_TWO);
vm.expectRevert();
multiSigTimelock.revokeConfirmation(txnId);
assertEq(multiSigTimelock.getTransaction(txnId).confirmations, 3);
}
}
Recommended Mitigation
After ensuring the account is actually a signer and it isn't the last signer for a transaction, add a check for if the signer has confirmed the transaction.
The signature will be then removed and the numberr of confirmations sees a decrement in value.
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();
}
//remove confirmation of signer if present
+ if (s_signatures[txnId][msg.sender]){
+ s_signatures[txnId][msg.sender] = false;
+ s_transactions[txnId].confirmations--;
}
// 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);
}
