Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/CoinbaseSmartWallet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ contract CoinbaseSmartWallet is ERC1271, IAccount, MultiOwnable, UUPSUpgradeable
}
}

constructor() {
constructor(address storageContract, bytes32 expectedCodeHash) MultiOwnable(storageContract, expectedCodeHash) {
// Implementation should not be initializable (does not affect proxies which use their own storage).
bytes[] memory owners = new bytes[](1);
owners[0] = abi.encode(address(0));
Expand Down
80 changes: 80 additions & 0 deletions src/ExternalWalletStorage.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;

/// @title External Wallet Storage
/// @notice Singleton contract for storing wallet state, protected by caller address and code hash
contract ExternalWalletStorage {
/// @notice Storage layout used by each wallet instance
struct WalletStorage {
uint256 nextOwnerIndex;
uint256 removedOwnersCount;
mapping(uint256 index => bytes owner) ownerAtIndex;
mapping(bytes bytes_ => bool isOwner_) isOwner;
}

/// @notice Maps (wallet address, wallet code hash) to wallet storage
mapping(address => mapping(bytes32 => WalletStorage)) private walletStorage;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice idea. but needs a small fix to be accepted by the bundler's validation rules:
In order to be associated with the account's address, the last mapping has to be the address, not the first.
so change the mapping to
mapping(bytes32 => mapping(address => WalletStorage))

another issue is allow an account to have arbitrary data stored in the external storage, but that is a separate issue.


/// @notice Access denied - caller must have expected code hash
error UnauthorizedAccess();

/// @dev Validates caller has expected code hash
function _validateCaller(bytes32 expectedCodeHash) internal view {
bytes32 callerCodeHash;
assembly {
callerCodeHash := extcodehash(caller())
}

if (callerCodeHash != expectedCodeHash) {
revert UnauthorizedAccess();
}
}

/// @notice Gets the next owner index
function getNextOwnerIndex(bytes32 expectedCodeHash) external view returns (uint256) {
_validateCaller(expectedCodeHash);
return walletStorage[msg.sender][expectedCodeHash].nextOwnerIndex;
}

/// @notice Gets the removed owners count
function getRemovedOwnersCount(bytes32 expectedCodeHash) external view returns (uint256) {
_validateCaller(expectedCodeHash);
return walletStorage[msg.sender][expectedCodeHash].removedOwnersCount;
}

/// @notice Gets owner at index
function getOwnerAtIndex(bytes32 expectedCodeHash, uint256 index) external view returns (bytes memory) {
_validateCaller(expectedCodeHash);
return walletStorage[msg.sender][expectedCodeHash].ownerAtIndex[index];
}

/// @notice Checks if bytes is an owner
function isOwner(bytes32 expectedCodeHash, bytes calldata ownerBytes) external view returns (bool) {
_validateCaller(expectedCodeHash);
return walletStorage[msg.sender][expectedCodeHash].isOwner[ownerBytes];
}

/// @notice Sets the next owner index
function setNextOwnerIndex(bytes32 expectedCodeHash, uint256 value) external {
_validateCaller(expectedCodeHash);
walletStorage[msg.sender][expectedCodeHash].nextOwnerIndex = value;
}

/// @notice Sets the removed owners count
function setRemovedOwnersCount(bytes32 expectedCodeHash, uint256 value) external {
_validateCaller(expectedCodeHash);
walletStorage[msg.sender][expectedCodeHash].removedOwnersCount = value;
}

/// @notice Sets owner at index
function setOwnerAtIndex(bytes32 expectedCodeHash, uint256 index, bytes calldata owner) external {
_validateCaller(expectedCodeHash);
walletStorage[msg.sender][expectedCodeHash].ownerAtIndex[index] = owner;
}

/// @notice Sets isOwner flag
function setIsOwner(bytes32 expectedCodeHash, bytes calldata ownerBytes, bool value) external {
_validateCaller(expectedCodeHash);
walletStorage[msg.sender][expectedCodeHash].isOwner[ownerBytes] = value;
}
}
94 changes: 34 additions & 60 deletions src/MultiOwnable.sol
Original file line number Diff line number Diff line change
@@ -1,42 +1,24 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;
pragma solidity ^0.8.23;

/// @notice Storage layout used by this contract.
///
/// @custom:storage-location erc7201:coinbase.storage.MultiOwnable
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 Maps index to owner bytes, used to idenfitied owners via a uint256 index.
///
/// Some uses—-such as signature validation for secp256r1 public key owners—-
/// requires the caller to assert the public key of the caller. To economize calldata,
/// we allow an index to identify an owner, so that the full owner bytes do
/// not need to be passed.
///
/// The `owner` bytes should either be
/// - An ABI encoded Ethereum address
/// - An ABI encoded public key
mapping(uint256 index => bytes owner) ownerAtIndex;
/// @dev Mapping of bytes to booleans indicating whether or not
/// bytes_ is an owner of this contract.
mapping(bytes bytes_ => bool isOwner_) isOwner;
}
import {ExternalWalletStorage} from "./ExternalWalletStorage.sol";

/// @title Multi Ownable
///
/// @notice Auth contract allowing multiple owners, each identified as bytes.
///
/// @author Coinbase (https://github.com/coinbase/smart-wallet)
contract MultiOwnable {
/// @dev Slot for the `MultiOwnableStorage` struct in storage.
/// Computed from
/// keccak256(abi.encode(uint256(keccak256("coinbase.storage.MultiOwnable")) - 1)) & ~bytes32(uint256(0xff))
/// Follows ERC-7201 (see https://eips.ethereum.org/EIPS/eip-7201).
bytes32 private constant MUTLI_OWNABLE_STORAGE_LOCATION =
0x97e2c6aad4ce5d562ebfaa00db6b9e0fb66ea5d8162ed5b243f51a2e03086f00;
/// @notice The external storage contract
ExternalWalletStorage public immutable STORAGE_CONTRACT;

/// @notice The expected code hash for authorization
bytes32 public immutable EXPECTED_CODE_HASH;

constructor(address storageContract, bytes32 expectedCodeHash) {
STORAGE_CONTRACT = ExternalWalletStorage(storageContract);
EXPECTED_CODE_HASH = expectedCodeHash;
}

/// @notice Thrown when the `msg.sender` is not an owner and is trying to call a privileged function.
error Unauthorized();
Expand Down Expand Up @@ -99,15 +81,19 @@ contract MultiOwnable {
///
/// @param owner The owner address.
function addOwnerAddress(address owner) external virtual onlyOwner {
_addOwnerAtIndex(abi.encode(owner), _getMultiOwnableStorage().nextOwnerIndex++);
uint256 nextIndex = nextOwnerIndex();
_addOwnerAtIndex(abi.encode(owner), nextIndex);
STORAGE_CONTRACT.setNextOwnerIndex(EXPECTED_CODE_HASH, nextIndex + 1);
}

/// @notice Adds a new public-key owner.
///
/// @param x The owner public key x coordinate.
/// @param y The owner public key y coordinate.
function addOwnerPublicKey(bytes32 x, bytes32 y) external virtual onlyOwner {
_addOwnerAtIndex(abi.encode(x, y), _getMultiOwnableStorage().nextOwnerIndex++);
uint256 nextIndex = nextOwnerIndex();
_addOwnerAtIndex(abi.encode(x, y), nextIndex);
STORAGE_CONTRACT.setNextOwnerIndex(EXPECTED_CODE_HASH, nextIndex + 1);
}

/// @notice Removes owner at the given `index`.
Expand Down Expand Up @@ -149,7 +135,7 @@ contract MultiOwnable {
///
/// @return `true` if the account is an owner else `false`.
function isOwnerAddress(address account) public view virtual returns (bool) {
return _getMultiOwnableStorage().isOwner[abi.encode(account)];
return STORAGE_CONTRACT.isOwner(EXPECTED_CODE_HASH, abi.encode(account));
}

/// @notice Checks if the given `x`, `y` public key is registered as owner.
Expand All @@ -159,7 +145,7 @@ contract MultiOwnable {
///
/// @return `true` if the account is an owner else `false`.
function isOwnerPublicKey(bytes32 x, bytes32 y) public view virtual returns (bool) {
return _getMultiOwnableStorage().isOwner[abi.encode(x, y)];
return STORAGE_CONTRACT.isOwner(EXPECTED_CODE_HASH, abi.encode(x, y));
}

/// @notice Checks if the given `account` bytes is registered as owner.
Expand All @@ -168,7 +154,7 @@ contract MultiOwnable {
///
/// @return `true` if the account is an owner else `false`.
function isOwnerBytes(bytes memory account) public view virtual returns (bool) {
return _getMultiOwnableStorage().isOwner[account];
return STORAGE_CONTRACT.isOwner(EXPECTED_CODE_HASH, account);
}

/// @notice Returns the owner bytes at the given `index`.
Expand All @@ -177,22 +163,21 @@ contract MultiOwnable {
///
/// @return The owner bytes (empty if no owner is registered at this `index`).
function ownerAtIndex(uint256 index) public view virtual returns (bytes memory) {
return _getMultiOwnableStorage().ownerAtIndex[index];
return STORAGE_CONTRACT.getOwnerAtIndex(EXPECTED_CODE_HASH, index);
}

/// @notice Returns the next index that will be used to add a new owner.
///
/// @return The next index that will be used to add a new owner.
function nextOwnerIndex() public view virtual returns (uint256) {
return _getMultiOwnableStorage().nextOwnerIndex;
return STORAGE_CONTRACT.getNextOwnerIndex(EXPECTED_CODE_HASH);
}

/// @notice Returns the current number of owners
///
/// @return The current owner count
function ownerCount() public view virtual returns (uint256) {
MultiOwnableStorage storage $ = _getMultiOwnableStorage();
return $.nextOwnerIndex - $.removedOwnersCount;
return nextOwnerIndex() - STORAGE_CONTRACT.getRemovedOwnersCount(EXPECTED_CODE_HASH);
}

/// @notice Tracks the number of owners removed
Expand All @@ -201,7 +186,7 @@ contract MultiOwnable {
///
/// @return The number of owners that have been removed.
function removedOwnersCount() public view virtual returns (uint256) {
return _getMultiOwnableStorage().removedOwnersCount;
return STORAGE_CONTRACT.getRemovedOwnersCount(EXPECTED_CODE_HASH);
}

/// @notice Initialize the owners of this contract.
Expand All @@ -211,8 +196,7 @@ contract MultiOwnable {
///
/// @param owners The initial set of owners.
function _initializeOwners(bytes[] memory owners) internal virtual {
MultiOwnableStorage storage $ = _getMultiOwnableStorage();
uint256 nextOwnerIndex_ = $.nextOwnerIndex;
uint256 nextIndex = nextOwnerIndex();
for (uint256 i; i < owners.length; i++) {
if (owners[i].length != 32 && owners[i].length != 64) {
revert InvalidOwnerBytesLength(owners[i]);
Expand All @@ -222,9 +206,10 @@ contract MultiOwnable {
revert InvalidEthereumAddressOwner(owners[i]);
}

_addOwnerAtIndex(owners[i], nextOwnerIndex_++);
_addOwnerAtIndex(owners[i], nextIndex);
nextIndex++;
}
$.nextOwnerIndex = nextOwnerIndex_;
STORAGE_CONTRACT.setNextOwnerIndex(EXPECTED_CODE_HASH, nextIndex);
}

/// @notice Adds an owner at the given `index`.
Expand All @@ -236,9 +221,8 @@ contract MultiOwnable {
function _addOwnerAtIndex(bytes memory owner, uint256 index) internal virtual {
if (isOwnerBytes(owner)) revert AlreadyOwner(owner);

MultiOwnableStorage storage $ = _getMultiOwnableStorage();
$.isOwner[owner] = true;
$.ownerAtIndex[index] = owner;
STORAGE_CONTRACT.setOwnerAtIndex(EXPECTED_CODE_HASH, index, owner);
STORAGE_CONTRACT.setIsOwner(EXPECTED_CODE_HASH, owner, true);

emit AddOwner(index, owner);
}
Expand All @@ -257,10 +241,9 @@ contract MultiOwnable {
revert WrongOwnerAtIndex({index: index, expectedOwner: owner, actualOwner: owner_});
}

MultiOwnableStorage storage $ = _getMultiOwnableStorage();
delete $.isOwner[owner];
delete $.ownerAtIndex[index];
$.removedOwnersCount++;
STORAGE_CONTRACT.setOwnerAtIndex(EXPECTED_CODE_HASH, index, "");
STORAGE_CONTRACT.setIsOwner(EXPECTED_CODE_HASH, owner, false);
STORAGE_CONTRACT.setRemovedOwnersCount(EXPECTED_CODE_HASH, removedOwnersCount() + 1);

emit RemoveOwner(index, owner);
}
Expand All @@ -275,13 +258,4 @@ contract MultiOwnable {

revert Unauthorized();
}

/// @notice Helper function to get a storage reference to the `MultiOwnableStorage` struct.
///
/// @return $ A storage reference to the `MultiOwnableStorage` struct.
function _getMultiOwnableStorage() internal pure returns (MultiOwnableStorage storage $) {
assembly ("memory-safe") {
$.slot := MUTLI_OWNABLE_STORAGE_LOCATION
}
}
}
Loading