-
Notifications
You must be signed in to change notification settings - Fork 83
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
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
93 changes: 93 additions & 0 deletions
93
contracts/smart-account/interfaces/modules/IMultiOwnedECDSAModule.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} |
225 changes: 225 additions & 0 deletions
225
contracts/smart-account/modules/MultiOwnedECDSAModule.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,225 @@ | ||
// 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)); | ||
unchecked { | ||
--numberOfOwners[msg.sender]; | ||
} | ||
} | ||
|
||
/// @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; | ||
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; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting :) is it cheaper?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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