Do not allow removeOwnerAtIndex to remove last owner, add function for removing last owner

.gas-snapshot

@@ -1,54 +1,68 @@
-AddOwnerAddressTest:testEmitsAddOwner() (gas: 92393)
-AddOwnerAddressTest:testIncreasesOwnerIndex() (gas: 90633)
-AddOwnerAddressTest:testRevertsIfAlreadyOwner() (gas: 93175)
+AddOwnerAddressTest:testEmitsAddOwner() (gas: 92417)
+AddOwnerAddressTest:testIncreasesOwnerIndex() (gas: 90691)
+AddOwnerAddressTest:testRevertsIfAlreadyOwner() (gas: 93210)
 AddOwnerAddressTest:testRevertsIfCalledByNonOwner() (gas: 11764)
-AddOwnerAddressTest:testSetsIsOwner() (gas: 90495)
-AddOwnerAddressTest:testSetsOwnerAtIndex() (gas: 98916)
-AddOwnerPublicKeyTest:testEmitsAddOwner() (gas: 115528)
-AddOwnerPublicKeyTest:testFuzzIsOwnerPublicKey(bytes32,bytes32) (runs: 256, μ: 115352, ~: 115352)
-AddOwnerPublicKeyTest:testRevertsIfAlreadyOwner() (gas: 116339)
+AddOwnerAddressTest:testSetsIsOwner() (gas: 90485)
+AddOwnerAddressTest:testSetsOwnerAtIndex() (gas: 98985)
+AddOwnerPublicKeyTest:testEmitsAddOwner() (gas: 115552)
+AddOwnerPublicKeyTest:testFuzzIsOwnerPublicKey(bytes32,bytes32) (runs: 256, μ: 115343, ~: 115343)
+AddOwnerPublicKeyTest:testRevertsIfAlreadyOwner() (gas: 116374)
 AddOwnerPublicKeyTest:testRevertsIfCalledByNonOwner() (gas: 11754)
-AddOwnerPublicKeyTest:testSetsIsOwner() (gas: 113794)
-AddOwnerPublicKeyTest:testSetsOwnerAtIndex() (gas: 128562)
-CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForDeployedAccount() (gas: 308670)
-CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForUndeployedAccount() (gas: 291699)
-CoinbaseSmartWalletFactoryTest:testDeployDeterministicPassValues() (gas: 267742)
-CoinbaseSmartWalletFactoryTest:test_CreateAccount_ReturnsPredeterminedAddress_WhenAccountAlreadyExists() (gas: 287406)
-CoinbaseSmartWalletFactoryTest:test_createAccountDeploysToPredeterminedAddress() (gas: 269093)
-CoinbaseSmartWalletFactoryTest:test_createAccountSetsOwnersCorrectly() (gas: 277981)
-CoinbaseSmartWalletFactoryTest:test_implementation_returnsExpectedAddress() (gas: 7676)
+AddOwnerPublicKeyTest:testSetsIsOwner() (gas: 113784)
+AddOwnerPublicKeyTest:testSetsOwnerAtIndex() (gas: 128631)
+CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForDeployedAccount() (gas: 308549)
+CoinbaseSmartWallet1271InputGeneratorTest:testGetReplaySafeHashForUndeployedAccount() (gas: 291578)
+CoinbaseSmartWalletFactoryTest:testDeployDeterministicPassValues() (gas: 267610)
+CoinbaseSmartWalletFactoryTest:test_CreateAccount_ReturnsPredeterminedAddress_WhenAccountAlreadyExists() (gas: 287363)
+CoinbaseSmartWalletFactoryTest:test_RevertsIfLength32ButLargerThanAddress() (gas: 318999)
+CoinbaseSmartWalletFactoryTest:test_createAccountDeploysToPredeterminedAddress() (gas: 268961)
+CoinbaseSmartWalletFactoryTest:test_createAccountSetsOwnersCorrectly() (gas: 277827)
+CoinbaseSmartWalletFactoryTest:test_exitIfAccountIsAlreadyInitialized() (gas: 268078)
+CoinbaseSmartWalletFactoryTest:test_implementation_returnsExpectedAddress() (gas: 7654)
+CoinbaseSmartWalletFactoryTest:test_initCodeHash() (gas: 7890)
 CoinbaseSmartWalletFactoryTest:test_revertsIfNoOwners() (gas: 29214)
-ERC1271Test:test_returnsExpectedDomainHashWhenProxy() (gas: 15384)
-ERC1271Test:test_static() (gas: 3057560)
-MultiOwnableInitializeTest:testRevertsIfLength32ButLargerThanAddress() (gas: 78927)
-MultiOwnableInitializeTest:testRevertsIfLength32NotAddress() (gas: 78908)
-MultiOwnableInitializeTest:testRevertsIfLengthNot32Or64() (gas: 101211)
-RemoveOwnerAtIndexTest:testEmitsRemoveOwner() (gas: 33270)
-RemoveOwnerAtIndexTest:testRemovesOwner() (gas: 32653)
-RemoveOwnerAtIndexTest:testRemovesOwnerAtIndex() (gas: 27538)
-RemoveOwnerAtIndexTest:testRevertsIfCalledByNonOwner() (gas: 11308)
-RemoveOwnerAtIndexTest:testRevertsIfNoOwnerAtIndex() (gas: 16624)
-TestCanSkipChainIdValidation:test_approvedSelectorsReturnTrue() (gas: 15933)
-TestCanSkipChainIdValidation:test_otherSelectorsReturnFalse() (gas: 12513)
-TestExecuteWithoutChainIdValidation:testExecute() (gas: 424907)
-TestExecuteWithoutChainIdValidation:testExecuteBatch() (gas: 729143)
-TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3450733, ~: 3582947)
-TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49053)
-TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49288)
-TestExecuteWithoutChainIdValidation:test_canChangeOwnerWithoutChainId() (gas: 287727)
-TestExecuteWithoutChainIdValidation:test_cannotCallExec() (gas: 220025)
+ERC1271Test:test_returnsExpectedDomainHashWhenProxy() (gas: 29176)
+ERC1271Test:test_static() (gas: 3180535)
+MultiOwnableInitializeTest:testRevertsIfLength32ButLargerThanAddress() (gas: 78972)
+MultiOwnableInitializeTest:testRevertsIfLength32NotAddress() (gas: 78953)
+MultiOwnableInitializeTest:testRevertsIfLengthNot32Or64() (gas: 101256)
+RemoveLastOwnerTest:test_emitsRemoveOwner() (gas: 50337)
+RemoveLastOwnerTest:test_removesOwner() (gas: 49609)
+RemoveLastOwnerTest:test_removesOwnerAtIndex() (gas: 49562)
+RemoveLastOwnerTest:test_revert_whenCalledByNonOwner(address) (runs: 256, μ: 19094, ~: 19094)
+RemoveLastOwnerTest:test_revert_whenNoOwnerAtIndex() (gas: 48379)
+RemoveLastOwnerTest:test_revert_whenWrongOwnerAtIndex() (gas: 34153)
+RemoveLastOwnerTest:test_reverts_whenNotLastOwner() (gas: 123427)
+RemoveOwnerAtIndexTest:test_emitsRemoveOwner() (gas: 55524)
+RemoveOwnerAtIndexTest:test_removesOwner() (gas: 54784)
+RemoveOwnerAtIndexTest:test_removesOwnerAtIndex() (gas: 54540)
+RemoveOwnerAtIndexTest:test_revert_whenCalledByNonOwner(address) (runs: 256, μ: 19145, ~: 19145)
+RemoveOwnerAtIndexTest:test_revert_whenNoOwnerAtIndex() (gas: 33372)
+RemoveOwnerAtIndexTest:test_revert_whenWrongOwnerAtIndex() (gas: 36694)
+RemoveOwnerAtIndexTest:test_reverts_ifIsLastOwner() (gas: 7470450)
+TestCanSkipChainIdValidation:test_approvedSelectorsReturnTrue() (gas: 15757)
+TestCanSkipChainIdValidation:test_otherSelectorsReturnFalse() (gas: 12425)
+TestExecuteWithoutChainIdValidation:testExecute() (gas: 424786)
+TestExecuteWithoutChainIdValidation:testExecuteBatch() (gas: 729088)
+TestExecuteWithoutChainIdValidation:testExecuteBatch(uint256) (runs: 256, μ: 3529376, ~: 3409593)
+TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49665)
+TestExecuteWithoutChainIdValidation:test__codesize() (gas: 49900)
+TestExecuteWithoutChainIdValidation:test_canChangeOwnerWithoutChainId() (gas: 287838)
+TestExecuteWithoutChainIdValidation:test_cannotCallExec() (gas: 12758)
 TestExecuteWithoutChainIdValidation:test_revertsIfCallerNotEntryPoint() (gas: 8598)
-TestExecuteWithoutChainIdValidation:test_revertsIfWrongNonceKey() (gas: 62275)
-TestExecuteWithoutChainIdValidation:test_revertsWithReservedNonce() (gas: 82324)
-TestInitialize:testInitialize() (gas: 21034)
-TestInitialize:test_cannotInitImplementation() (gas: 2714468)
+TestExecuteWithoutChainIdValidation:test_revertsIfWrongNonceKey() (gas: 62298)
+TestExecuteWithoutChainIdValidation:test_revertsWithReservedNonce() (gas: 82370)
+TestImplementation:testImplementation() (gas: 12601)
+TestInitialize:testInitialize() (gas: 21100)
+TestInitialize:test_cannotInitImplementation() (gas: 2836084)
 TestIsValidSignature:testReturnsInvalidIfPasskeySigButWrongOwnerLength() (gas: 39469)
 TestIsValidSignature:testRevertsIfEthereumSignatureButWrongOwnerLength() (gas: 24040)
-TestIsValidSignature:testSmartWalletSigner() (gas: 2990421)
+TestIsValidSignature:testRevertsIfOwnerIsInvalidEthereumAddress() (gas: 22021)
+TestIsValidSignature:testSmartWalletSigner() (gas: 3111896)
 TestIsValidSignature:testValidateSignatureWithEOASigner() (gas: 24922)
 TestIsValidSignature:testValidateSignatureWithEOASignerFailsWithWrongSigner() (gas: 23877)
 TestIsValidSignature:testValidateSignatureWithPasskeySigner() (gas: 421256)
 TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsBadOwnerIndex() (gas: 34958)
 TestIsValidSignature:testValidateSignatureWithPasskeySignerFailsWithWrongBadSignature() (gas: 428722)
-TestUpgradeToAndCall:testUpgradeToAndCall() (gas: 25477)
-TestValidateUserOp:testValidateUserOp() (gas: 447275)
+TestUpgradeToAndCall:testUpgradeToAndCall() (gas: 25522)
+TestValidateUserOp:testValidateUserOp() (gas: 447366)

certora/specs/ERC4337AccountInv.spec

@@ -43,8 +43,15 @@ invariant notTheSameOwnerAgain(uint256 i, uint256 j)
     {
         preserved removeOwnerAtIndex(uint256 index, bytes owner) with (env e2) {
             require i == index;
-            bytes empty;
-            require empty.length == 0;
-            require !isOwnerBytes(empty);
+            bytes empty1;
+            require empty1.length == 0;
+            require !isOwnerBytes(empty1);
+        }
+
+        preserved removeLastOwner(uint256 index, bytes owner) with (env e3) {
+            require i == index;
+            bytes empty2;
+            require empty2.length == 0;
+            require !isOwnerBytes(empty2);
         }
     }

src/MultiOwnable.sol

@@ -7,6 +7,8 @@ pragma solidity ^0.8.4;
 struct MultiOwnableStorage {
     /// @dev Tracks the index of the next owner to add.
     uint256 nextOwnerIndex;
+    /// @dev Tracks number of owners that have been removed.
+    uint256 removedOwnersCount;
     /// @dev Mapping of indices to raw owner bytes, used to idenfitied owners by their
     ///      uint256 id.
     ///
@@ -52,8 +54,9 @@ contract MultiOwnable {
     /// @notice Thrown when `owner` argument does not match owner found at index.
     ///
     /// @param index The index of the owner to be removed.
-    /// @param owner Owner to be removed which did not match owner found at index.
-    error WrongOwnerAtIndex(uint256 index, bytes owner);
+    /// @param expectedOwner The owner passed in the remove call.
+    /// @param actualOwner The actual owner at `index`.
+    error WrongOwnerAtIndex(uint256 index, bytes expectedOwner, bytes actualOwner);
 
     /// @notice Thrown when trying to intialize the contracts owners if a provided owner is neither
     ///         64 bytes long (for passkey) nor a valid address.
@@ -67,6 +70,14 @@ contract MultiOwnable {
     /// @param owner The invalid raw abi encoded owner bytes.
     error InvalidEthereumAddressOwner(bytes owner);
 
+    /// @notice Thrown when removeOwnerAtIndex is called and there is only one current owner.
+    error LastOwner();
+
+    /// @notice Thrown when removeLastOwner is called and there is more than one current owner.
+    ///
+    /// @param ownersRemaining The number of current owners.
+    error NotLastOwner(uint256 ownersRemaining);
+
     /// @notice Emitted when a new owner is registered.
     ///
     /// @param index The owner index.
@@ -88,33 +99,51 @@ contract MultiOwnable {
     /// @notice Convenience function to add a new owner address.
     ///
     /// @param owner The owner address.
-    function addOwnerAddress(address owner) public virtual onlyOwner {
+    function addOwnerAddress(address owner) external virtual onlyOwner {
         _addOwner(abi.encode(owner));
     }
 
     /// @notice Convenience function to add a new owner passkey.
     ///
     /// @param x The owner public key x coordinate.
     /// @param y The owner public key y coordinate.
-    function addOwnerPublicKey(bytes32 x, bytes32 y) public virtual onlyOwner {
+    function addOwnerPublicKey(bytes32 x, bytes32 y) external virtual onlyOwner {
         _addOwner(abi.encode(x, y));
     }
 
-    /// @notice Removes an owner from the given `index`.
+    /// @notice Removes owner at the given `index`.
     ///
-    /// @dev Reverts if the provided owner is not found at `index`.
+    /// @dev Reverts if the owner is not registered at `index`.
+    /// @dev Reverts if there is currently only one owner.
+    /// @dev Reverts if `owner` does not match bytes found at `index`.
     ///
-    /// @param index The index to remove from.
-    /// @param owner_ The owner at the specific index.
-    function removeOwnerAtIndex(uint256 index, bytes calldata owner_) public virtual onlyOwner {
-        bytes memory owner = ownerAtIndex(index);
-        if (owner.length == 0) revert NoOwnerAtIndex(index);
-        if (keccak256(owner) != keccak256(owner_)) revert WrongOwnerAtIndex(index, owner_);
+    /// @param index The index of the owner to be removed.
+    /// @param owner The ABI encoded bytes of the owner to be removed.
+    function removeOwnerAtIndex(uint256 index, bytes calldata owner) external virtual onlyOwner {
+        MultiOwnableStorage storage $ = _getMultiOwnableStorage();
+        if ($.nextOwnerIndex - $.removedOwnersCount == 1) {
+            revert LastOwner();
+        }
+
+        _removeOwnerAtIndex(index, owner);
+    }
 
-        delete _getMultiOwnableStorage().isOwner[owner];
-        delete _getMultiOwnableStorage().ownerAtIndex[index];
+    /// @notice Removes owner at the given `index`, which should be the only current owner.
+    ///
+    /// @dev Reverts if the owner is not registered at `index`.
+    /// @dev Reverts if there is currently more than one owner.
+    /// @dev Reverts if `owner` does not match bytes found at `index`.
+    ///
+    /// @param index The index of the owner to be removed.
+    /// @param owner The ABI encoded bytes of the owner to be removed.
+    function removeLastOwner(uint256 index, bytes calldata owner) external virtual onlyOwner {
+        MultiOwnableStorage storage $ = _getMultiOwnableStorage();
+        uint256 ownersRemaining = $.nextOwnerIndex - $.removedOwnersCount;
+        if (ownersRemaining > 1) {
+            revert NotLastOwner(ownersRemaining);
+        }
 
-        emit RemoveOwner(index, owner);
+        _removeOwnerAtIndex(index, owner);
     }
 
     /// @notice Checks if the given `account` address is registered as owner.
@@ -161,6 +190,13 @@ contract MultiOwnable {
         return _getMultiOwnableStorage().nextOwnerIndex;
     }
 
+    /// @notice Tracks the number of owners removed, used with nextOwnerIndex to avoid removing all owners
+    ///
+    /// @return The number of owners that have been removed.
+    function removedOwnersCount() public view virtual returns (uint256) {
+        return _getMultiOwnableStorage().removedOwnersCount;
+    }
+
     /// @notice Initialize the owners of this contract.
     ///
     /// @dev Intended to be called when the smart account is first deployed.
@@ -197,12 +233,35 @@ contract MultiOwnable {
     function _addOwnerAtIndex(bytes memory owner, uint256 index) internal virtual {
         if (isOwnerBytes(owner)) revert AlreadyOwner(owner);
 
-        _getMultiOwnableStorage().isOwner[owner] = true;
-        _getMultiOwnableStorage().ownerAtIndex[index] = owner;
+        MultiOwnableStorage storage $ = _getMultiOwnableStorage();
+        $.isOwner[owner] = true;
+        $.ownerAtIndex[index] = owner;
 
         emit AddOwner(index, owner);
     }
 
+    /// @notice Removes owner at the given `index`.
+    ///
+    /// @dev Reverts if the owner is not registered at `index`.
+    /// @dev Reverts if `owner` does not match bytes found at `index`.
+    ///
+    /// @param index The index of the owner to be removed.
+    /// @param owner The ABI encoded bytes of the owner to be removed.
+    function _removeOwnerAtIndex(uint256 index, bytes calldata owner) internal virtual {
+        bytes memory owner_ = ownerAtIndex(index);
+        if (owner_.length == 0) revert NoOwnerAtIndex(index);
+        if (keccak256(owner_) != keccak256(owner)) {
+            revert WrongOwnerAtIndex({index: index, expectedOwner: owner, actualOwner: owner_});
+        }
+
+        MultiOwnableStorage storage $ = _getMultiOwnableStorage();
+        delete $.isOwner[owner];
+        delete $.ownerAtIndex[index];
+        $.removedOwnersCount++;
+
+        emit RemoveOwner(index, owner);
+    }
+
     /// @notice Checks if the sender is an owner of this contract or the contract itself.
     ///
     /// @dev Revert if the sender is not an owner fo the contract itself.

test/CoinbaseSmartWallet/IsValidSignature.t.sol

@@ -166,14 +166,15 @@ contract TestIsValidSignature is SmartWalletTestBase {
         account.isValidSignature(hash, abi.encode(CoinbaseSmartWallet.SignatureWrapper(1, signature)));
     }
 
-    // @dev this case should not be possible, but we need to explicitly test the revert case
+    /// @dev this case should not be possible, but we need to explicitly test the revert case
     function testRevertsIfOwnerIsInvalidEthereumAddress() public {
         bytes32 hash = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5;
         bytes32 toSign = account.replaySafeHash(hash);
         (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, toSign);
         bytes memory signature = abi.encodePacked(r, s, v);
         bytes32 invalidAddress = bytes32(uint256(type(uint160).max) + 1);
-        bytes32 slot_ownerAtIndex = 0x97e2c6aad4ce5d562ebfaa00db6b9e0fb66ea5d8162ed5b243f51a2e03086f01; // MUTLI_OWNABLE_STORAGE_LOCATION + 1
+        bytes32 slot_ownerAtIndex =
+            bytes32(uint256(0x97e2c6aad4ce5d562ebfaa00db6b9e0fb66ea5d8162ed5b243f51a2e03086f00) + 2); // MUTLI_OWNABLE_STORAGE_LOCATION + 2
         bytes32 slot_ownerAtIndex_zeroIndex =
             bytes32(uint256(keccak256(abi.encodePacked(keccak256(abi.encode(0, slot_ownerAtIndex))))));
         vm.store(address(account), slot_ownerAtIndex_zeroIndex, invalidAddress);

test/MultiOwnable/RemoveLastOwner.t.sol

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.23;
+
+import "./RemoveOwnerBase.t.sol";
+
+contract RemoveLastOwnerTest is RemoveOwnerBaseTest {
+    function setUp() public override {
+        owners.push(owner1Bytes);
+        mock.init(owners);
+        index = 0;
+        ownerToRemove = owner1Bytes;
+    }
+
+    function test_reverts_whenNotLastOwner() public {
+        vm.prank(owner1Address);
+        mock.addOwnerPublicKey("x", "y");
+        vm.expectRevert(abi.encodeWithSelector(MultiOwnable.NotLastOwner.selector, 2));
+        _removeOwner();
+    }
+
+    function _removeOwner() internal override {
+        vm.prank(owner1Address);
+        mock.removeLastOwner(index, ownerToRemove);
+    }
+}