feat: use openzeppelin's eip712 and change authorization name #453

pakim249CAL · 2023-02-01T18:59:03Z

Pull Request

Issue(s) fixed

This pull request:

fixes Reconsider EIP712 implementation #452

To preemptively answer some questions that may arise:

The reason that EIP712Upgradeable is not used is because this version stores some useless storage variables, namely the hashes of the name and version.

EIP712 should function identically despite it being the non-upgradeable variant. There are no storage variables in this contract, and the domain separator is built entirely through immutable logic. It uses address(this), thus satisfying the property that the domain separator is built using the proxy contract's address as context.

I've also removed much of the signature logic in favor of using the ECDSA library's system of checks.

The reason that I renamed the authorization typehash is because I believe the standard is for each function that utilizes EIP712 to have its own unique typehash. Thus the previous naming makes little sense if we ever introduce a new function that utilizes EIP712.

src/libraries/Constants.sol

src/MorphoStorage.sol

Rubilmax · 2023-02-02T14:48:04Z

src/MorphoStorage.sol

+    constructor(address addressesProvider, uint8 eModeCategoryId)
+        EIP712(
+            Constants.EIP712_NAME,
+            string(abi.encodePacked(Constants.EIP712_VERSION, ".", Strings.toString(eModeCategoryId)))


Suggested change

string(abi.encodePacked(Constants.EIP712_VERSION, ".", Strings.toString(eModeCategoryId)))

string.concat(Constants.EIP712_VERSION, ".", Strings.toString(eModeCategoryId))

Does this string represent the version argument of the instance ? Why do we need to add the E-mode id?

Yes

I don't think there's a need, but it's weird to share the same id between 2 instances of Morpho

#453 (comment)

I don't think it's strictly required because the address of the proxy is used as part of the domain separator. But I believe it is good practice to maintain distinct identities to each contract. We could instead put this in an arbitrary salt, but I don't think this is necessary. It should suffice to have different versions for each e-mode category.

Tristan22400 · 2023-02-02T16:26:36Z

src/Morpho.sol

-        if (signature.v != 27 && signature.v != 28) revert Errors.InvalidValueV();
+        bytes32 structHash =
+            keccak256(abi.encode(Constants.APPROVE_MANAGER_TYPEHASH, delegator, manager, isAllowed, nonce, deadline));
+        bytes32 digest = _hashTypedDataV4(structHash);


why did you remove the checks on v and s ?

These checks are performed in OZ's ECDSA tryRecover

Tristan22400 · 2023-02-02T16:31:28Z

src/MorphoStorage.sol

+    constructor(address addressesProvider, uint8 eModeCategoryId)
+        EIP712(
+            Constants.EIP712_NAME,
+            string(abi.encodePacked(Constants.EIP712_VERSION, ".", Strings.toString(eModeCategoryId)))


Does this string represent the version argument of the instance ? Why do we need to add the E-mode id?

Tristan22400 · 2023-02-02T16:36:29Z

src/libraries/Constants.sol

-
-    uint256 internal constant MAX_VALID_ECDSA_S = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0; // The highest valid value for s in an ECDSA signature pair (0 < s < secp256k1n ÷ 2 + 1)
+    bytes32 internal constant APPROVE_MANAGER_TYPEHASH =
+        keccak256("ApproveManager(address delegator,address manager,bool isAllowed,uint256 nonce,uint256 deadline)"); // The EIP-712 typehash for approveManagerBySig Authorization.


I do not understand why change approve manager typehash from EIP712 Authorization typehash

I think it is for clarity, because this type hash is only used upon manager approval

Correct. AFAIK it is standard practice for each function which has a signature method to be the same as the method to be called. My goal here is consistency, not much opinion on the naming.

MerlinEgalite · 2023-02-02T18:26:58Z

src/MorphoStorage.sol

@@ -46,7 +47,7 @@ abstract contract MorphoStorage is Initializable, Ownable2StepUpgradeable, EIP71
    /// @param addressesProvider The address of the pool addresses provider.
    /// @param eModeCategoryId The e-mode category of the deployed Morpho. 0 for the general mode.
    constructor(address addressesProvider, uint8 eModeCategoryId)
-        EIP712(Constants.EIP712_NAME, Constants.EIP712_VERSION)
+        EIP712(Constants.EIP712_NAME, string.concat(Constants.EIP712_VERSION, ".", Strings.toString(eModeCategoryId)))


what's the bytecode of adding this?

clearly too much. investigating.

pakim249CAL · 2023-02-02T20:49:41Z

Too much bytecode, so I'm ruling out OZ's implementation for now. Closing.

Rubilmax · 2023-02-03T08:25:48Z

Reopening because I don't understand the conclusion: I just compiled the contracts and it seems we're loosing ~1kb because of OZ's implementation. Do you have the same number? If yes, why do you consider this "too much bytecode"?
Please note there's still a 2kb margin after this change (3kb before)

Pushing this because we've just made the error of trying to rewrite OZ's standard and it led us to introduce a bug. Perhaps we should really consider base our work on standards, even if it costs bytecode.

pakim249CAL added 2 commits February 1, 2023 13:57

feat: use openzeppelin's eip712 and change authorization name

eb2cbb4

chore: properly capitalize typehash input

4a41bc7

pakim249CAL requested review from MerlinEgalite, Rubilmax and Tristan22400 February 1, 2023 19:01

chore: clean up constants

d109a1a

pakim249CAL linked an issue Feb 1, 2023 that may be closed by this pull request

Reconsider EIP712 implementation #452

Closed

pakim249CAL added 2 commits February 1, 2023 20:43

refactor: use non-upgradeable eip712 to not use storage vars

4c59808

refactor: siimplify approve with sig

b1c2dd3

pakim249CAL requested review from QGarchery and MathisGD February 2, 2023 02:03

MerlinEgalite requested changes Feb 2, 2023

View reviewed changes

src/libraries/Constants.sol Outdated Show resolved Hide resolved

Rubilmax reviewed Feb 2, 2023

View reviewed changes

src/MorphoStorage.sol Outdated Show resolved Hide resolved

fix: use emode category id in version and remove unused constant

1ba1a1a

pakim249CAL requested review from MerlinEgalite and Rubilmax February 2, 2023 14:11

Rubilmax previously approved these changes Feb 2, 2023

View reviewed changes

Tristan22400 reviewed Feb 2, 2023

View reviewed changes

chore: use string concat

686b997

pakim249CAL dismissed Rubilmax’s stale review via 686b997 February 2, 2023 17:38

pakim249CAL requested review from Rubilmax and Tristan22400 February 2, 2023 17:38

MerlinEgalite reviewed Feb 2, 2023

View reviewed changes

Merge remote-tracking branch 'origin/dev' into fix/domain-separator

01b5a0c

pakim249CAL closed this Feb 2, 2023

Rubilmax reopened this Feb 3, 2023

Rubilmax marked this pull request as draft February 3, 2023 08:25

Rubilmax mentioned this pull request Feb 3, 2023

Consider where we want each function to exist #170

Closed

pakim249CAL mentioned this pull request Feb 3, 2023

refactor EIP712 #458

Merged

Rubilmax closed this Feb 6, 2023

MerlinEgalite deleted the fix/domain-separator branch May 3, 2023 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use openzeppelin's eip712 and change authorization name #453

feat: use openzeppelin's eip712 and change authorization name #453

pakim249CAL commented Feb 1, 2023 •

edited

Rubilmax Feb 2, 2023

Tristan22400 Feb 2, 2023

Rubilmax Feb 2, 2023

pakim249CAL Feb 2, 2023

Tristan22400 Feb 2, 2023

Rubilmax Feb 2, 2023 •

edited

Tristan22400 Feb 2, 2023

Tristan22400 Feb 2, 2023

Rubilmax Feb 2, 2023

pakim249CAL Feb 2, 2023

MerlinEgalite Feb 2, 2023

pakim249CAL Feb 2, 2023

pakim249CAL commented Feb 2, 2023

Rubilmax commented Feb 3, 2023 •

edited

	string(abi.encodePacked(Constants.EIP712_VERSION, ".", Strings.toString(eModeCategoryId)))
	string.concat(Constants.EIP712_VERSION, ".", Strings.toString(eModeCategoryId))

feat: use openzeppelin's eip712 and change authorization name #453

feat: use openzeppelin's eip712 and change authorization name #453

Conversation

pakim249CAL commented Feb 1, 2023 • edited

Pull Request

Issue(s) fixed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rubilmax Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakim249CAL commented Feb 2, 2023

Rubilmax commented Feb 3, 2023 • edited

pakim249CAL commented Feb 1, 2023 •

edited

Rubilmax Feb 2, 2023 •

edited

Rubilmax commented Feb 3, 2023 •

edited