-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Add ERC7739 and ERC7739Utils #5243
Changes from 77 commits
6d4cbbf
6dc7749
efa9e2b
211770b
1a25df6
fae755e
353f278
33b0c1b
a013306
90fffbf
913e522
01e19a1
8030e42
81b0caa
d83ed9e
d6767b9
8aecee3
3a1efa4
c6b5aac
b50eb21
d1bc68d
866be61
4dab8d0
bbb5271
93b6892
cb961c5
99c0b7c
7934421
75d470e
a194289
8be86a7
77f9821
a183548
176f151
21e950b
6a8cfde
71f8ec8
7ac541a
8341a4f
8bac09c
c2e9839
09a5691
6216b5c
afbb6e4
2a50933
5bb9757
cd3e388
bdf55bb
b72e3b1
0a2fb48
2073799
bda0f5f
7927a33
0a3c4ef
75377ee
8bb0438
7129f38
7873042
c65e137
b9696bd
cdae537
4d109f8
3224ee6
2ac663f
d592192
dd6caea
b3cef39
22e4216
c2e2ef7
6626d3e
8b9ede9
05ea45e
422889b
21f08cd
1786b69
2f6f154
8fa48f9
f3111a5
8f244fa
c3b61dc
79af185
c0fa4b7
cfdb514
2702222
dbe84c2
893f680
8dfe87e
391c92c
48211e4
211bb38
00aef29
8a33b80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'openzeppelin-solidity': minor | ||
--- | ||
|
||
`ERC7739Utils`: Add a library that implements a defensive rehashing mechanism to prevent replayability of smart contract signatures based on the ERC-7739. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'openzeppelin-solidity': minor | ||
--- | ||
|
||
`ERC7739Signer`: An abstract contract to validate signatures following the rehashing scheme from `ERC7739Utils`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.20; | ||
|
||
import {ERC7739Signer} from "../utils/cryptography/draft-ERC7739Signer.sol"; | ||
import {EIP712} from "../utils/cryptography/EIP712.sol"; | ||
import {ECDSA} from "../utils/cryptography/ECDSA.sol"; | ||
|
||
contract ERC7739SignerECDSAMock is ERC7739Signer { | ||
address private immutable _signer; | ||
|
||
constructor(address signerAddr) EIP712("ERC7739SignerECDSA", "1") { | ||
_signer = signerAddr; | ||
} | ||
|
||
function _validateSignature(bytes32 hash, bytes calldata signature) internal view virtual override returns (bool) { | ||
(address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, signature); | ||
return _signer == recovered && err == ECDSA.RecoverError.NoError; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.20; | ||
|
||
import {ERC7739Signer} from "../utils/cryptography/draft-ERC7739Signer.sol"; | ||
import {EIP712} from "../utils/cryptography/EIP712.sol"; | ||
import {P256} from "../utils/cryptography/P256.sol"; | ||
|
||
contract ERC7739SignerP256Mock is ERC7739Signer { | ||
bytes32 private immutable _qx; | ||
bytes32 private immutable _qy; | ||
|
||
constructor(bytes32 qx, bytes32 qy) EIP712("ERC7739SignerP256", "1") { | ||
_qx = qx; | ||
_qy = qy; | ||
} | ||
|
||
function _validateSignature(bytes32 hash, bytes calldata signature) internal view virtual override returns (bool) { | ||
bytes32 r = bytes32(signature[0x00:0x20]); | ||
bytes32 s = bytes32(signature[0x20:0x40]); | ||
return P256.verify(hash, r, s, _qx, _qy); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.20; | ||
|
||
import {ERC7739Signer} from "../utils/cryptography/draft-ERC7739Signer.sol"; | ||
import {EIP712} from "../utils/cryptography/EIP712.sol"; | ||
import {RSA} from "../utils/cryptography/RSA.sol"; | ||
|
||
contract ERC7739SignerRSAMock is ERC7739Signer { | ||
bytes private _e; | ||
bytes private _n; | ||
|
||
constructor(bytes memory e, bytes memory n) EIP712("ERC7739SignerRSA", "1") { | ||
_e = e; | ||
_n = n; | ||
} | ||
|
||
function _validateSignature(bytes32 hash, bytes calldata signature) internal view virtual override returns (bool) { | ||
return RSA.pkcs1Sha256(hash, signature, _e, _n); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.20; | ||
|
||
import {IERC1271} from "../../interfaces/IERC1271.sol"; | ||
import {EIP712} from "./EIP712.sol"; | ||
import {MessageHashUtils} from "./MessageHashUtils.sol"; | ||
import {ERC7739Utils} from "./draft-ERC7739Utils.sol"; | ||
import {ShortStrings} from "../ShortStrings.sol"; | ||
|
||
/** | ||
* @dev Validates signatures wrapping the message hash in a nested EIP712 type. See {ERC7739Utils}. | ||
* | ||
* Linking the signature to the EIP-712 domain separator is a security measure to prevent signature replay across different | ||
* EIP-712 domains (e.g. a single offchain owner of multiple contracts). | ||
* | ||
* This contract requires implementing the {_validateSignature} function, which passes the wrapped message hash, | ||
* which may be either an typed data or a personal sign nested type. | ||
* | ||
* NOTE: {EIP712} uses {ShortStrings} to optimize gas costs for short strings (up to 31 characters). | ||
* Consider that strings longer than that will use storage, which may limit the ability of the signer to | ||
* be used within the ERC-4337 validation phase (due to ERC-7562 storage access rules). | ||
*/ | ||
abstract contract ERC7739Signer is EIP712, IERC1271 { | ||
using ERC7739Utils for *; | ||
|
||
/** | ||
* @dev Attempts validating the signature in a nested EIP-712 type. | ||
* | ||
* A nested EIP-712 type might be presented in 2 different ways: | ||
* | ||
* - As a nested EIP-712 typed data | ||
* - As a _personal_ signature (an EIP-712 mimic of the `eth_personalSign` for a smart contract) | ||
*/ | ||
function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4 result) { | ||
return _isValidSignature(hash, signature) ? IERC1271.isValidSignature.selector : bytes4(0xffffffff); | ||
} | ||
|
||
/** | ||
* @dev Internal version of {isValidSignature} that returns a boolean. | ||
*/ | ||
function _isValidSignature(bytes32 hash, bytes calldata signature) internal view virtual returns (bool) { | ||
return | ||
_isValidNestedPersonalSignSignature(hash, signature) || _isValidNestedTypedDataSignature(hash, signature); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear to me whether this matches the spec: https://ercs.ethereum.org/ERCS/erc-7739#signature-verification-workflow-deduction
|
||
} | ||
|
||
/** | ||
* @dev Nested personal signature verification. | ||
*/ | ||
function _isValidNestedPersonalSignSignature( | ||
bytes32 hash, | ||
bytes calldata signature | ||
) internal view virtual returns (bool) { | ||
return _validateSignature(_domainSeparatorV4().toNestedPersonalSignHash(hash), signature); | ||
} | ||
|
||
/** | ||
* @dev Nested EIP-712 typed data verification. | ||
*/ | ||
function _isValidNestedTypedDataSignature( | ||
bytes32 hash, | ||
bytes calldata signature | ||
) internal view virtual returns (bool) { | ||
// decode signature | ||
(bytes calldata nestedSignature, bytes32 separator, bytes32 contents, bytes calldata contentsType) = signature | ||
.decodeSignature(); | ||
|
||
// fetch domain details | ||
( | ||
, | ||
string memory name, | ||
string memory version, | ||
, | ||
address verifyingContract, | ||
bytes32 salt, | ||
uint256[] memory extensions | ||
) = eip712Domain(); | ||
|
||
// Check that hash is the correct nested typed data hash, and that the nested signature is valid. | ||
return | ||
hash == | ||
ERC7739Utils.toNestedTypedDataHash( | ||
separator, | ||
ERC7739Utils.typedDataNestedStructHash( | ||
contentsType, | ||
contents, | ||
name, | ||
version, | ||
verifyingContract, | ||
salt, | ||
extensions | ||
) | ||
) && | ||
_validateSignature(hash, nestedSignature); | ||
} | ||
|
||
/** | ||
* @dev Signature validation algorithm. | ||
* | ||
* WARNING: Implementing a signature validation algorithm is a security-sensitive operation as it involves | ||
* cryptographic verification. It is important to review and test thoroughly before deployment. Consider | ||
* using one of the signature verification libraries ({ECDSA}, {P256} or {RSA}). | ||
*/ | ||
function _validateSignature(bytes32 hash, bytes calldata signature) internal view virtual returns (bool); | ||
} |
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.
Is this accurate? The account is able to read its own storage during validation so I don't think there will be any issues.
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.
That is in case account A relies on another other contract B for validation:
Callstack is something like:
handleOps
validateUserOp
.Lets say that is implemented as a multisig that, so it will interpret the signature as a
bytes[]
and parse it. For each sub-signature, it will verify it against an identity. Identity could be an EOA, but it could also be another contract that implements ERC-1271. Under the hood, the ERC-1271 may do a further multisig, or use P256/RSA as a signing schemeisValidSignature
: This is where the identity contract might use ERC-7739, because it has control over multiple ERC-4337 Accounts. In that case, there are storage restrictions that apply.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.
@frangio would that be good ?
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 but this is a much larger issue than EIP-712 name and version strings, because the public keys will usually be in storage.
IMO mentioning this here is a distraction.
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.
There are ways to put the public keys outside of storage (for example using a Proxy with immutable args) ... but the dependency on ERC712 prevents is just too much.
I don't have a strong opinion either way :/
I'm considering merging with (or without) the message for the audit, and have PR with all the proposed changes (in this case adding or removing) that we'll discuss in parallele of the audit.