Do not allow removeOwnerAtIndex to remove last owner, add function for removing last owner #43
Conversation
| @@ -82,31 +92,47 @@ 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 { | |||
There was a problem hiding this comment.
other clean up I am grouping in here
There was a problem hiding this comment.
I'm wondering if this is overly restrictive. I could imagine a plugin/module that might help users manage owners. Depending on how the community agrees on module/plugin support, this call could be executed in the context of this contract.
There was a problem hiding this comment.
I assume we would do an upgrade to enable this? So we could change.
| @@ -189,12 +222,30 @@ 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(); | |||
There was a problem hiding this comment.
wilsoncusack
force-pushed
the
wilson/last-owner-guard
branch
from
ffc9170
to
683e1b8
Compare
| 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); |
There was a problem hiding this comment.
@stevieraykatz this felt nice to have to me
There was a problem hiding this comment.
Agreed -- could help track down malicious replays
src/MultiOwnable.sol
Outdated
| /// @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 remove. |
There was a problem hiding this comment.
| /// @param index The index of the owner to be remove. | |
| /// @param index The index of the owner to be removed. |
| 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); |
There was a problem hiding this comment.
Agreed -- could help track down malicious replays
| @@ -82,31 +92,47 @@ 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 { | |||
There was a problem hiding this comment.
I'm wondering if this is overly restrictive. I could imagine a plugin/module that might help users manage owners. Depending on how the community agrees on module/plugin support, this call could be executed in the context of this contract.
wilsoncusack
changed the title
Adds a guard on removeOwnerAtIndex such that it will revert if the call will leave no owners on the account.
No tests yet because still considering. Removes the ability to renounce ownership, except by setting some like 0xdead owner.