[FEATURE] Nexus to allow for vanilla ERC-1271 flow

filmakarov commented 2 months ago

Feature or Improvement Description

Improves backwards compatibility

Benefits & Outcomes

I realised that some protocols may already have their eip-712 data structs secure, in a way they already include all the sensitive data (smart account address etc) in the data struct that is hashed. Thus such protocols do not need to use erc-7739. So we should allow those protocols to access the vanilla erc-1271 flow.

Potential options are:

Expose Nexus.isValidSIgnatureLegacy() interface
Inside Nexus.isValidSignature method try calling both IValidator.isValidSignatureWithSender and IERC1271Legacy.isValidSignatureWithSenderLegacy methods

IMO, between those two, the second option is preferable as it is backwards compatible even for protocols that are not aware of ERC7739. So in any case, Nexus.IsValidSignature is called. Then:

If the hash is erc7739 hash and the IValidator supports it, it will successfully check the signature
If the hash is vanilla 1271 hash and the IValidator doesn't support 7739 , it will successfully check the signature
If the hash is vanilla 1271 hash and the IValidator expects 7739 via isValidSignatureWithSender, it will return 0xffffffff and proceed to try isValidSignatureWithSenderLegacy, which will successfully check that signature
If the hash is 7739 hash and IValidator doesn't support it, IValidator won't be able to process the hash and check the signature, but that's unlikely as most default validators on the market will likely support 7739.

There's also third option.

Nexus pretends to know nothing about 7739 and always calls IValidator.isValidSignatureWithSender() as per ERC-7579.
Then, it's IValidator's job to try both 7739 (for signTypedData and personal_sign) and vanilla 1271 (for secure typed data structs or just protocols that do not care)

Then, for K1Validator it will mean isValidSignatureWithSender becomes:

function isValidSignatureWithSender(address, bytes32 hash, bytes calldata data) external view returns (bytes4) {
        (bytes32 computeHash, bytes calldata truncatedSignature) = _erc1271HashForIsValidSignatureViaNestedEIP712(hash, data);
       if (_validateSignatureForOwner(smartAccountOwners[msg.sender], computeHash, truncatedSignature)) {
           return ERC1271_MAGICVALUE
       } else {
           return _validateSignatureForOwner(smartAccountOwners[msg.sender], hash, data) ? ERC1271_MAGICVALUE : ERC1271_INVALID;
    }

I think, the third one is the most consistent one, as it doesn't put any additional expectations on an IValidator module, but having isValidSignatureWithSender required by ERC-7579 itself. Then it's up to a module to support (like K1 Validator) or not support (like Smart Sessions) the vanilla erc-1271.

Code of Conduct

[X] I agree to follow this project's Code of Conduct.

livingrockrises commented 2 months ago

possibly can go with third option, it also seems least amount of code change. I am in favour, Given today itself we open PR and show SLOC changes and get a yes from auditors (I see you pasted issue on discord already) btw cmichel is not part of this audit now.

filmakarov commented 2 months ago

https://github.com/bcnmy/nexus/pull/169

bcnmy / nexus