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.
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:
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:
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'sisOwner
mapping will be set tofalse
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 onlycan 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 theowners
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 sameisOwner
mapping, so to speak. So whenever there's a for loop looping through theowners
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 theowners
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:
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++)
for (uint256 i = 0; i < owners.length - 1; i++)
Using test function:
Test result: