Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi Owned ECDSA Validation Module #151

Merged
merged 6 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from 5 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

/**
* @title ECDSA ownership Authorization module for Biconomy Smart Accounts.
* @dev Compatible with Biconomy Modular Interface v 0.1
* - It allows to validate user operations signed by EOA private key.
* - EIP-1271 compatible (ensures Smart Account can validate signed messages).
* - One owner per Smart Account.
* - Does not support outdated eth_sign flow for cheaper validations
* (see https://support.metamask.io/hc/en-us/articles/14764161421467-What-is-eth-sign-and-why-is-it-a-risk-)
* !!!!!!! Only EOA owners supported, no Smart Account Owners
* For Smart Contract Owners check SmartContractOwnership module instead
* @author Fil Makarov - <[email protected]>
*/
interface IMultiOwnedECDSAModule {
event OwnershipTransferred(
address indexed smartAccount,
address indexed oldOwner,
address indexed newOwner
);

error AlreadyInitedForSmartAccount(address smartAccount);
error WrongSignatureLength();
error NotEOA(address account);
error ZeroAddressNotAllowedAsOwner();
error OwnerAlreadyUsedForSmartAccount(address owner, address smartAccount);
error NotAnOwner(address owner, address smartAccount);

/**
* @dev Initializes the module for a Smart Account.
* Should be used at a time of first enabling the module for a Smart Account.
* @param eoaOwners The owner of the Smart Account. Should be EOA!
*/
function initForSmartAccount(
address[] calldata eoaOwners
) external returns (address);

/**
* @dev Sets/changes an for a Smart Account.
* Should be called by Smart Account itself.
* @param owner The current owner of the Smart Account to be replaced.
* @param newOwner The new owner of the Smart Account.
*/
function transferOwnership(address owner, address newOwner) external;

/**
* @dev Adds owner for Smart Account.
* should be called by Smart Account.
* @param owner The owner of the Smart Account.
*/
function addOwner(address owner) external;

/**
* @dev Renounces ownership from owner
* should be called by Smart Account.
* @param owner The owner to be removed
*/
function removeOwner(address owner) external;

/**
* @dev Returns the if the address provided is one of owners of the Smart Account.
* @param smartAccount Smart Account address.
* @param eoaAddress The address to check for ownership
*/
function isOwner(
address smartAccount,
address eoaAddress
) external view returns (bool);

/**
* @dev Returns the number of owners of the Smart Account.
* @param smartAccount Smart Account address.
* @return The number of owners of the Smart Account.
*/
function getNumberOfOwners(
address smartAccount
) external view returns (uint256);

/**
* @dev Validates a signature for a message signed by address.
* @dev Also try dataHash.toEthSignedMessageHash()
* @param dataHash hash of the data
* @param moduleSignature Signature to be validated.
* @param smartAccount expected signer Smart Account address.
* @return EIP1271_MAGIC_VALUE if signature is valid, 0xffffffff otherwise.
*/
function isValidSignatureForAddress(
bytes32 dataHash,
bytes memory moduleSignature,
address smartAccount
) external view returns (bytes4);
}
223 changes: 223 additions & 0 deletions contracts/smart-account/modules/MultiOwnedECDSAModule.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

/* solhint-disable no-unused-import */

import {BaseAuthorizationModule} from "./BaseAuthorizationModule.sol";
import {EIP1271_MAGIC_VALUE} from "contracts/smart-account/interfaces/ISignatureValidator.sol";
import {UserOperation} from "@account-abstraction/contracts/interfaces/UserOperation.sol";
import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {IMultiOwnedECDSAModule} from "../interfaces/modules/IMultiOwnedECDSAModule.sol";
import {IAuthorizationModule} from "../interfaces/IAuthorizationModule.sol";
import {ISignatureValidator} from "../interfaces/ISignatureValidator.sol";

/**
* @title ECDSA ownership Authorization module for Biconomy Smart Accounts.
* @dev Compatible with Biconomy Modular Interface v 0.1
* - It allows to validate user operations signed by EOA private key.
* - EIP-1271 compatible (ensures Smart Account can validate signed messages).
* - One owner per Smart Account.
* - Does not support outdated eth_sign flow for cheaper validations
* (see https://support.metamask.io/hc/en-us/articles/14764161421467-What-is-eth-sign-and-why-is-it-a-risk-)
* !!!!!!! Only EOA owners supported, no Smart Account Owners
* For Smart Contract Owners check SmartContractOwnership module instead
* @author Fil Makarov - <[email protected]>
*/

contract MultiOwnedECDSAModule is
BaseAuthorizationModule,
IMultiOwnedECDSAModule
{
using ECDSA for bytes32;

string public constant NAME = "ECDSA Ownership Registry Module";
string public constant VERSION = "0.2.0";

// owner => smartAccount => isOwner
mapping(address => mapping(address => bool)) internal _smartAccountOwners;
mapping(address => uint256) internal numberOfOwners;

/// @inheritdoc IMultiOwnedECDSAModule
function initForSmartAccount(
address[] calldata eoaOwners
) external returns (address) {
if (numberOfOwners[msg.sender] != 0) {
revert AlreadyInitedForSmartAccount(msg.sender);
}
uint256 ownersToAdd = eoaOwners.length;
for (uint256 i; i < ownersToAdd; ) {
if (eoaOwners[i] == address(0))
revert ZeroAddressNotAllowedAsOwner();
if (_smartAccountOwners[eoaOwners[i]][msg.sender])
revert OwnerAlreadyUsedForSmartAccount(
eoaOwners[i],
msg.sender
);

_smartAccountOwners[eoaOwners[i]][msg.sender] = true;
unchecked {
++i;
}
}
numberOfOwners[msg.sender] = ownersToAdd;
return address(this);
}

/// @inheritdoc IMultiOwnedECDSAModule
function transferOwnership(
address owner,
address newOwner
) external override {
if (_isSmartContract(newOwner)) revert NotEOA(owner);
if (newOwner == address(0)) revert ZeroAddressNotAllowedAsOwner();
if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
if (owner == newOwner)
revert OwnerAlreadyUsedForSmartAccount(newOwner, msg.sender);
if (!_smartAccountOwners[owner][msg.sender])
revert NotAnOwner(owner, msg.sender);
if (_smartAccountOwners[newOwner][msg.sender])
revert OwnerAlreadyUsedForSmartAccount(newOwner, msg.sender);
_transferOwnership(msg.sender, owner, newOwner);
}

/// @inheritdoc IMultiOwnedECDSAModule
function addOwner(address owner) external override {
if (_isSmartContract(owner)) revert NotEOA(owner);
if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
if (_smartAccountOwners[owner][msg.sender])
revert OwnerAlreadyUsedForSmartAccount(owner, msg.sender);

_smartAccountOwners[owner][msg.sender] = true;
unchecked {
++numberOfOwners[msg.sender];
}
}

/// @inheritdoc IMultiOwnedECDSAModule
function removeOwner(address owner) external override {
if(!_smartAccountOwners[owner][msg.sender])
revert NotAnOwner(owner, msg.sender);
_transferOwnership(msg.sender, owner, address(0));
--numberOfOwners[msg.sender];
filmakarov marked this conversation as resolved.
Show resolved Hide resolved
}

/// @inheritdoc IMultiOwnedECDSAModule
function isOwner(
address smartAccount,
address eoaAddress
) external view override returns (bool) {
return _smartAccountOwners[eoaAddress][smartAccount];
}

/// @inheritdoc IAuthorizationModule
function validateUserOp(
UserOperation calldata userOp,
bytes32 userOpHash
) external view virtual override returns (uint256) {
(bytes memory cleanEcdsaSignature, ) = abi.decode(
userOp.signature,
(bytes, address)
);
if (_verifySignature(userOpHash, cleanEcdsaSignature, userOp.sender)) {
return VALIDATION_SUCCESS;
}
return SIG_VALIDATION_FAILED;
}

/**
* @inheritdoc ISignatureValidator
* @dev Validates a signature for a message.
* To be called from a Smart Account.
* @param dataHash Exact hash of the data that was signed.
* @param moduleSignature Signature to be validated.
* @return EIP1271_MAGIC_VALUE if signature is valid, 0xffffffff otherwise.
*/
function isValidSignature(
bytes32 dataHash,
bytes memory moduleSignature
) public view virtual override returns (bytes4) {
return
isValidSignatureForAddress(dataHash, moduleSignature, msg.sender);
}

/// @inheritdoc IMultiOwnedECDSAModule
function isValidSignatureForAddress(
bytes32 dataHash,
bytes memory moduleSignature,
address smartAccount
) public view virtual override returns (bytes4) {
if (_verifySignature(dataHash, moduleSignature, smartAccount)) {
return EIP1271_MAGIC_VALUE;
}
return bytes4(0xffffffff);
}

/// @inheritdoc IMultiOwnedECDSAModule
function getNumberOfOwners(
address smartAccount
) public view returns (uint256) {
return numberOfOwners[smartAccount];
}

/**
* @dev Transfers ownership for smartAccount and emits an event
* @param newOwner Smart Account address.
*/
function _transferOwnership(
address smartAccount,
address owner,
address newOwner
) internal {
_smartAccountOwners[owner][smartAccount] = false;
_smartAccountOwners[newOwner][smartAccount] = true;
Comment on lines +173 to +174
Copy link
Contributor

Choose a reason for hiding this comment

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

could be swapped if the first value is always true and the second is always false:
similarly to this

uint256 a = 1;
uint256 b = 2;

(a, b) = (b, a);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interesting :) is it cheaper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if not, I would suggest not doing this as in this exact case it makes things less readable

(_smartAccountOwners[owner][smartAccount], _smartAccountOwners[newOwner][smartAccount]) = (_smartAccountOwners[newOwner][smartAccount], _smartAccountOwners[owner][smartAccount]);

looks bulky

Copy link
Contributor

Choose a reason for hiding this comment

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

yes it's cheaper but agree that it makes it very difficult to read 😅 let's discuss when we will focus on optimization

emit OwnershipTransferred(smartAccount, owner, newOwner);
}

/**
* @dev Validates a signature for a message.
* @dev Check if signature was made over dataHash.toEthSignedMessageHash() or just dataHash
* The former is for personal_sign, the latter for the typed_data sign
* Only EOA owners supported, no Smart Account Owners
* For Smart Contract Owners check SmartContractOwnership Module instead
* @param dataHash Hash of the data to be validated.
* @param signature Signature to be validated.
* @param smartAccount expected signer Smart Account address.
* @return true if signature is valid, false otherwise.
*/
function _verifySignature(
bytes32 dataHash,
bytes memory signature,
address smartAccount
) internal view returns (bool) {
if (signature.length < 65) revert WrongSignatureLength();
address recovered = (dataHash.toEthSignedMessageHash()).recover(
signature
);
if (
recovered != address(0) &&
_smartAccountOwners[recovered][smartAccount]
) {
return true;
}
recovered = dataHash.recover(signature);
if (
recovered != address(0) &&
_smartAccountOwners[recovered][smartAccount]
) {
return true;
}
return false;
}

/**
* @dev Checks if the address provided is a smart contract.
* @param account Address to be checked.
*/
function _isSmartContract(address account) internal view returns (bool) {
uint256 size;
assembly {
size := extcodesize(account)
}
return size > 0;
}
}
Loading