Active Boosts

29286 - [SC - Medium] MultiSigWalletremoveOwner - L The bug allows th...

MultiSigWallet::removeOwner() - L127: The bug allows the last owner in the owners array to remain in the array even after being marked as not an owner if they are the one intended for removal.

Submitted on Mar 13th 2024 at 09:20:08 UTC by @OxSCSamurai for Boost | ZeroLend

Report ID: #29286

Report type: Smart Contract

Report severity: Medium

Target: https://github.com/zerolend/governance

Impacts:

  • Manipulation of governance voting result deviating from voted outcome and resulting in a direct change from intended effect of original results

Description

Brief/Intro

MultiSigWallet::removeOwner() - L127: The bug allows the last owner in the owners array to remain in the array even after being marked as not an owner if they are the one intended for removal.

  • Although this is a serious bug, it's not obvious to me which impact in scope fits best here. Again, I believe the available impacts in scope are limiting and could compromise protocol security. This bug report is one example of a very serious bug which doesn't seem to have an obvious impact in scope match. This is something for Immunefi team to look at and try improve, because I could have easily decided submitting this bug report isn't worth my time and effort. But I know better and helping secure web3 is top priority, therefore I've pulled all the strings in an effort to make at least one impact in scope fit well enough.

But alas, heed my advice, take this bug report seriously, as the bug has a tendency to mutate into a critical level monster over time...

Vulnerability Details

SUMMARY:

The buggy function, slightly modified so that I didnt need to deal with onlyWallet access control during tests:

    /// @dev Allows to remove an owner. Transaction has to be sent by wallet.
    /// @param owner Address of owner.
    //function removeOwner(address owner) public onlyWallet ownerExists(owner) {
    function removeOwner(address owner) public ownerExists(owner) { 
        isOwner[owner] = false;
        for (uint256 i = 0; i < owners.length - 1; i++)
            if (owners[i] == owner) {
                owners[i] = owners[owners.length - 1];
                owners.pop();
                break;
            }
        if (required > owners.length) changeRequirement(owners.length);
        emit OwnerRemoval(owner);
    }

The bug in the removeOwner() function of the ZeroLend protocol's multisignature wallet contract allows the last owner in the owners array to remain in the array even after being marked as not an owner, if they are the one intended for removal. This occurs because the loop condition in the function stops iterating before reaching the last owner, resulting in their continued & permanent presence in the array.

The bug in this function lies within the loop condition:

for (uint256 i = 0; i < owners.length - 1; i++)

This loop will iterate over all owners except the last one due to the combo of i < and owners.length - 1. If the owner to be removed happens to be the last one in the owners array, this loop will not process it, resulting in the owner not being removed from the array.

NOTE:

  • Regardless of whether this bug poses an immediate threat/risk to the protocol or not, it is almost certainly guaranteed to pose a critical level risk eventually if the bug is not fixed, simply because each time an owner is removed who happened to be in the last position in the owners array, the problem compounds.

  • In other words, the longer the bug is left unfixed the worse the problem will get over time until it becomes a critical level problem where the owners array usage is DoS'ed along with ability to confirm transactions, which is one of the core functionalities of the multisig. The ability to confirm transactions will be permanently DoS'ed...

That's it, PoC done, critical bug proven, can I have my pizza now please? 👀

Not so fast Samurai! ⚔️

Impact Details

POTENTIAL IMPACTS IN SCOPE:

  • Manipulation of governance voting result deviating from voted outcome and resulting in a direct change from intended effect of original results

  • Temporary freezing of funds for at least 1 hour

  • ?

IMPACTS:

  • the immediate and obvious impact is that any owner to be removed that's in the last position of the owners array will be permanently stuck in the array, even when it's isOwner mapping will be set to false successfully, so he wont be able to take any governance/signer actions at least, but can NEVER remove this owner from the array.

  • will be able to add incompletely removed owner back to the owners array but as a separate & duplicate array entry only

  • can affect accuracy of results coming from getConfirmationCount()

  • over time it's possible(almost guaranteed) for required to become greater than the available usable owners in the owners array

Some possible worst case scenario impacts:

  • it's possible for same owner/signer to double vote/confirm, and is enabled by: unsuccessful removal + add same owner back again, resulting in both entries in owners array linked to the same isOwner mapping, so to speak. So whenever there's a for loop looping through the owners array, this owner's contribution(vote/confirmation) will be double-counted/double-processed, which could be tantamount to unintentional governance vote result manipulation.

  • DoS of owners array usability and therefore DoS of ability to vote/confirm transactions: what makes this possible is when all owners have been removed when each of them were the last entry in the owners array, and this continued until the owners array reached a point where the max limit of 50 owners/confirmations(I need to confirm this?) were reached, then its impossible to add new owners or replace existing owners, while all existing owners are unusable and not replaceable, resulting in permanent DoS of critical multisig functionality.

References

https://github.com/zerolend/governance/blob/84d48522cdb14f4a5a4aefc4059c0ea1e2e97afa/contracts/governance/MultiSigWallet.sol#L127

Proof of Concept

PROOF OF CONCEPT (PoC):

TEST 1: Proving that the bug exists, and that my bugfix works TEST 2: Proving that the unsuccessfully removed owner can never be fully removed ever again, nor replaced TEST 3: Proving it's possible for same owner to get duplicated in owners array, enabling double voting/confirmation counts TEST 4: Proving that the accuracy of results from isConfirmed() and getConfirmationCount() can be affected

These two tests I will do PoC for later, will add to this bug report once ready: TEST 5: Proving it's possible to permanently DoS owners array and ability to vote/confirm transactions TEST 6: Proving it's possible for required to become greater than the number of usable owners/signers

TESTS:

TEST 1: Proving that the bug exists, and that my bugfix works

Foundry based test contract used for this test:

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.23 <0.9.0;

import "forge-std/Test.sol";

import "governance/contracts/governance/MultiSigWallet.sol";

contract TestMultisigRemoveOwner is Test {
    MultiSigWallet multiSigWallet;

    function setUp() public {
        address[] memory initialOwners = new address[](3);
        // Populate initialOwners with appropriate addresses
        initialOwners[0] = address(0x1234567890123456789012345678901234567890);
        initialOwners[1] = address(0x5234567890123456789012345678901234567899);
        initialOwners[2] = address(0x9999999999999999999999999999999999999999);

        uint256 requiredConfirmations = 2;

        // Deploy the MultiSigWallet contract
        multiSigWallet = new MultiSigWallet(initialOwners, requiredConfirmations);
    }

    function test_removeOwner() external {

        address[] memory _owners = multiSigWallet.getOwners();
        // Try to remove the last owner in the array
        multiSigWallet.removeOwner(_owners[_owners.length - 1]);

        // If not removed, array length should still be 3
        assert(multiSigWallet.getOwners().length == 3); // if this passes, then owner in last position of `owners` array, was not removed.
        // If removed, array length should be 2
        //assert(multiSigWallet.getOwners().length == 2); // if this passes, then owner in last position of `owners` array, was successfully removed.

    }
}

Note: obviously alternate between commenting out the relevant assert statements above when running the tests for the bug unfixed and bug fixed.

Test results: bug not fixed: for (uint256 i = 0; i < owners.length - 1; i++)

Using test function:

    function test_removeOwner() external {

        address[] memory _owners = multiSigWallet.getOwners();
        // Try to remove the last owner in the array
        multiSigWallet.removeOwner(_owners[_owners.length - 1]);

        // If not removed, array length should still be 3
        assert(multiSigWallet.getOwners().length == 3); // if this passes, then owner in last position of `owners` array, was not removed.
        // If removed, array length should be 2
        //assert(multiSigWallet.getOwners().length == 2); // if this passes, then owner in last position of `owners` array, was successfully removed.
    }

Test result:

$ forge test --contracts test/TestMultisigRemoveOwner.t.sol --mt test_removeOwner -vvvvv
[⠒] Compiling...
[⠢] Compiling 1 files with 0.8.23
[⠆] Solc 0.8.23 finished in 1.96s
Compiler run successful!

Ran 1 test for test/TestMultisigRemoveOwner.t.sol:TestMultisigRemoveOwner
[PASS] test_removeOwner() (gas: 24758)
Traces:
  [1499074] TestMultisigRemoveOwner::setUp()
    ├─ [1442244] → new MultiSigWallet@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← 6302 bytes of code
    └─ ← ()