The owner of MultiSigWallet can also be replaced through above process. However, the owner could be replaced to address(0) if somehow the owners made a mistake. Additionally, if the total number of owners equals to threshold required before incorrect owner replacing, there would be no enough qualified owners to correct the mistake after the transaction was executed.
Impact
MultiSigWallet will be inoperable
Any assets in MultiSigWallet will be locked forever
Risk Breakdown
Difficulty to Exploit: Hard
Recommendation
Check if newOwner is address(0):
function replaceOwner(address owner, address newOwner) public onlyWallet ownerExists(owner) ownerDoesNotExist(newOwner)+ notNull(newOwner) { for (uint i=0; i<owners.length; i++) if (owners[i] == owner) { owners[i] = newOwner; break; } isOwner[owner] = false; isOwner[newOwner] = true; OwnerRemoval(owner); OwnerAddition(newOwner); }
Run forge test --fork-url MAINNET_URL --chain-id 1 --fork-block-number 18620472 --match-test testUpdateOwner(replace MAINNET_URL with your Ethereum RPC URL to test:
// SPDX-License-Identifier: UNLICENSEDpragmasolidity ^0.8.13;import {Test, console} from"forge-std/Test.sol";interface IMultiSigWallet { function submitTransaction(address destination, uint value, bytes memory data) external returns (uint transactionId);
functionconfirmTransaction(uint transactionId) external;functionrevokeConfirmation(uint transactionId) external;functionexecuteTransaction(uint transactionId) external;functiongetOwners() externalreturns (address[] memory);functionrequired() externalreturns (uint);functionisOwner(address owner) externalreturns (bool);}contractMultiSigWalletTestisTest { IMultiSigWallet wallet;functionsetUp() public { wallet =IMultiSigWallet(0x2028834B2c0A36A918c10937EeA71BE4f932da52); }functiontestUpdateOwner() public {address[] memory owners = wallet.getOwners();assertEq(owners.length,6);assertEq(wallet.required(),4);address destination =address(wallet);uint value =0;bytesmemory removeOwnerData1 = abi.encodeWithSignature("removeOwner(address)", owners[4]);bytesmemory removeOwnerData2 = abi.encodeWithSignature("removeOwner(address)", owners[5]);//remove 2 owners vm.startPrank(owners[0]);uint transactionId1 = wallet.submitTransaction(destination, value, removeOwnerData1);uint transactionId2 = wallet.submitTransaction(destination, value, removeOwnerData2); vm.stopPrank();for (uint i=1; i<4; i++) { vm.startPrank(owners[i]); wallet.confirmTransaction(transactionId1); wallet.confirmTransaction(transactionId2); vm.stopPrank(); } owners = wallet.getOwners();//the total number of owners is 4, equals to the threshold `required`assertEq(owners.length,4);assertEq(wallet.required(),4);//replace the last owner to address(0)bytesmemory replaceOwnerData = abi.encodeWithSignature("replaceOwner(address,address)",owners[3],address(0)); vm.prank(owners[0]);uint transactionId = wallet.submitTransaction(destination, value, replaceOwnerData);for (uint i=1; i<4; i++) { vm.prank(owners[i]); wallet.confirmTransaction(transactionId); } //the last owner became address(0), threshold i still 4 and only 3 available owners left.afterwards the threshold can never be met
owners = wallet.getOwners();assertEq(owners.length,4);assertEq(wallet.required(),4);assertEq(owners[3],address(0));assertEq(wallet.isOwner(address(0)),true); }}