Submission Details
Severity:
Revoked signers does not revoke already signed transaction
Revoked signers does not revoked already signed transaction
Description
-
When the admin a signer role it does not revoke the signed transactions from this specific users
-
In that case, instead of having 3 needed transactions, we only need 2 from two different signers to be able to execute the transaction.
In the revokeSigningRole, we only revoke the role, not the already proposed and signed transactions. In the case we want to revoke a signers, it either means the key was compromise or the transactions are suspiscious. In that case, we should revoke this user and also the associated state to keep the same number of active signers for a proposal.
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();
}
// 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:
-
A signers has submitted a proposal or has confirmed a user proposal.
-
Can happened when the admin revoke the signer
Impact:
Impact the state of the multisig as the signers should no more impact previous nor active proposals.
Proof of Concept
function testRevokeSignerShouldRevokeConfirmation() public {
// ARRANGE
vm.deal(OWNER, OWNER_BALANCE_ONE);
vm.prank(OWNER);
multiSigTimelock.grantSigningRole(SIGNER_TWO);
// ACT & ASSERT
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_ONE, hex"");
console2.log("This is the first transaction ID", txnId);
assertEq(txnId, 0);
// Signers confirmed the proposal
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
// Proposal should have one confirmation
MultiSigTimelock.Transaction memory proposedTxn = multiSigTimelock.getTransaction(txnId);
assertEq(proposedTxn.confirmations, 1);
// Admin revoke signer role
vm.prank(OWNER);
multiSigTimelock.revokeSigningRole(SIGNER_TWO);
// Proposal should now have zero confirmations
proposedTxn = multiSigTimelock.getTransaction(txnId);
assertEq(proposedTxn.confirmations, 0);
}
[287412] MultiSigTimeLockTest::testRevokeSignerShouldRevokeConfirmation()
├─ [0] VM::deal(MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 500000000000000000 [5e17])
│ └─ ← [Return]
├─ [0] VM::prank(MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [83181] MultiSigTimelock::grantSigningRole(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ ├─ emit RoleGranted(role: 0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, account: signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa], sender: MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Stop]
├─ [0] VM::prank(MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [103447] MultiSigTimelock::proposeTransaction(spender_one: [0x488dE584674d70E8Da70C42Cd4CC73Cd63d3dB23], 500000000000000000 [5e17], 0x)
│ ├─ emit TransactionProposed(transactionId: 0, to: spender_one: [0x488dE584674d70E8Da70C42Cd4CC73Cd63d3dB23], value: 500000000000000000 [5e17])
│ └─ ← [Return] 0
├─ [0] console::log("This is the first transaction ID", 0) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::assertEq(0, 0) [staticcall]
│ └─ ← [Return]
├─ [0] VM::prank(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ └─ ← [Return]
├─ [49353] MultiSigTimelock::confirmTransaction(0)
│ ├─ emit TransactionConfirmed(transactionId: 0, signer: signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ └─ ← [Stop]
├─ [3173] MultiSigTimelock::getTransaction(0) [staticcall]
│ └─ ← [Return] Transaction({ to: 0x488dE584674d70E8Da70C42Cd4CC73Cd63d3dB23, value: 500000000000000000 [5e17], data: 0x, confirmations: 1, proposedAt: 1, executed: false })
├─ [0] VM::assertEq(1, 1) [staticcall]
│ └─ ← [Return]
├─ [0] VM::prank(MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [11873] MultiSigTimelock::revokeSigningRole(signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa])
│ ├─ emit RoleRevoked(role: 0x8d12c52fe7c286acb23e8b6fad43ba0a7b1bdee59ad6818c044e478cc487c15b, account: signer_two: [0x884D93fF97A786C23d91993E20382Fc4C26Bc2aa], sender: MultiSigTimeLockTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Stop]
├─ [3173] MultiSigTimelock::getTransaction(0) [staticcall]
│ └─ ← [Return] Transaction({ to: 0x488dE584674d70E8Da70C42Cd4CC73Cd63d3dB23, value: 500000000000000000 [5e17], data: 0x, confirmations: 1, proposedAt: 1, executed: false })
├─ [0] VM::assertEq(1, 0) [staticcall]
│ └─ ← [Revert] assertion failed: 1 != 0
└─ ← [Revert] assertion failed: 1 != 0
Recommended Mitigation
Track the active transactions and revoke the user signature on that one. Notice that you may want to implement another design approach as in the case you have too much active transactions, this tx could not beexecuted as it could run out of gas.
function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
/// ...
/// Revoke previous approval on this signer
+ for (uint256 i = 0; i < activeTxns; ++i) {
+ if (s_signatures[i][_account]) {
+ delete s_signatures[i][_account];
+ }
+ }
}
Updates
Lead Judging Commences
kelechikizito 4 days ago
Submission Judgement Published Assigned finding tags:
Stale Confirmation Vulnerability/Ghost Voting Issue
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.