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

Friendly Metatransaction Signing #1300

Open
area opened this issue Oct 3, 2024 · 7 comments
Open

Friendly Metatransaction Signing #1300

area opened this issue Oct 3, 2024 · 7 comments

Comments

@area
Copy link
Member

area commented Oct 3, 2024

It is a truth universally acknowledged, that a single user in possession of a good fortune, must be in want of a human-readable message to sign when making metatransactions.

Currently, when signing metatransactions, we show the user an unintelligble blob:

Screenshot 2024-10-03 at 16 26 35

A sufficiently savvy user could build the transaction they're expecting to encode, hash it, and compare it to this one, but practically no-one is going to do that. It's also scary for non-technical users to sign something they don't understand. To solve both of these, we want intelligble signing messages that will look something like this:

Screenshot 2024-10-03 at 13 21 04

The way this example was achieved was with EIP712. Realistically, I don't believe there is another way - certainly not as widely supported as EIP712 which, in a regime where we are attempting to support as many different wallets as possible, seems like an essential property. Alternatives might be our own Metamask Snap in a similar vein to this one, or the Sourcify Metamask Snap in conjunction with validating our contracts - but these require the user installing extra components, and don't work outside Metamask.

I also believe that it is essential that the human-readable elements of this are directly used to execute the transaction. Including a hex blob that is used to execute the transaction in addition to human readable elements achieves nothing, in my mind. The human readable elements must be validated on chain, and either proved they are consistent with the transaction to be sent, or be used to call the function in question directly.

Per-function implementation

What is the cost of implementing this? Unfortunately, I believe quite high. Our current metatransaction implementation adds a single function callMetaTransaction which can be used to call any function on the contracts via metatransactions. It is my current belief that to support EIP712, we would need to implement a separate function for each function we want to call via metatransactions - so the interface would become something like:


  struct EIP712Signature {
    bytes32 r;
    bytes32 s;
    uint8 v;
    address signer;
    uint256 deadline;
  }


  function someFunction(uint256 arg1, address arg2) external;
  function someFunctionWithSignature(uint256 arg1, address arg2, EIP712Signature signature) external;

where here, I've used a custom data type for the signature to reduce the number of arguments that need to be added to the function call (which potentially represents a real issue for functions where we are close to the stack limit). The latter function, after confirming the signature was valid according to EIP712, would call someFunction with the arguments provided and indicating it is a metatransaction in the same way the current metatransaction implementation does, with msgSender() returning the address of the signer.

There remains a small chance that requiring an extra function is not true for the contracts - there is a possibility that our function arguments are simple enough that a functional-enough equivalent of abi.encode could be written to handle all of them. I intend to explore this possibility, but I think this is unlikely. If it were true now, it might also not be true in the future as functions are added. It will also surely be gassier than a per-function implementation.

Implementing functions on a case-by-case basis would mostly be rote work, once the first few implementations were done, but we have a lot of functions. It would also mean that new versions of the contracts, with new functions, would require more work than they do currently (as every function would need to be implemented twice).

ChainId

With the multi-chain functionality, there is a desire for users to not have to switch chains in order to sign metatransactions. On the face of it, EIP712 represents an issue on that front. From the specification of the DomainSeparator:

uint256 chainId the EIP-155 chain id. The user-agent should refuse signing if it does not match the currently active chain.

(emphasis not mine). This is the case, at least, for Metamask (I have checked). However, the specification for the domain separator also says:

... the type of eip712Domain is a struct named EIP712Domain with one or more of the below fields.

(emphasis mine). So we can, in fact, leave out the chainId field from the domain separator. This is not ideal, but it is a workaround that would allow us to use EIP712 without requiring users to switch chains. The intention behind the chainId field is to prevent replay attacks, but I believe we can instead use the salt field, set to the chainId, to achieve the same effect. This would mean Metamask would not refuse to sign the message (I have tested this to be true), but it would still (I believe) not allow cross-chain replays. This is a non-standard use of the field (and would mean that, for example, we could not use the OpenZeppelin implementation of EIP712), but I can't see a (security-related) reason why we can't do this.

Conclusion

None of the above is set in stone, it merely represents my current understanding of the situation but I thought it would be useful to get this in place. I am open to suggestions on directions to explore, but given the amount of work that I now believe this will require (including on the frontend), I thought it best to get visibility on it as early as possible.

All that being said, it is my current belief that EIP712 support represents the best way to achieve these friendly metatransactions, despite the amount of work that it would appear to represent.

@arrenv
Copy link
Member

arrenv commented Oct 3, 2024

Thank you @area for the detailed breakdown, very helpful in providing some clarity.

Going with the assumption that this is comprehensive and the likely best/only solution, and aside from doing all the work required now and in future, here are my initial thoughts on this:

  • We go with the approach as you have described but only for specific transactions, such as for pivotal user onboarding transactions e.g. creating a colony, making a payment, finalizing a motion.
  • We provide a user friendly way of comparing the blob in the frontend, e.g. providing the transaction details, encoding and expected hash to compare it, within the transaction list or just the transaction details provide links to resources where they can do it themselves.
  • We do a combination of both of these.

Question, it seems Metatransactions is the main cause of the work and limitations here, are there other alternatives to Metatransactions that could make this easier? Such as using account abstraction with EIP-3074 or the incoming EIP-4337?

@area
Copy link
Member Author

area commented Oct 4, 2024

There remains a small chance that requiring an extra function is not true for the contracts - there is a possibility that our function arguments are simple enough that a functional-enough equivalent of abi.encode could be written to handle all of them. I intend to explore this possibility, but I think this is unlikely. If it were true now, it might also not be true in the future as functions are added. It will also surely be gassier than a per-function implementation.

After toying with this a bit, I don't think this is feasible. In general, this is the same conclusion that others have come to. There are some functions that I think it could work for, but it would come at a cost to the user experience - I think we would have to include all the permission proofs in the human-readable message, which is not necessary with a per-function implementation where we can restrict ourselves to what actually matters to the user.

Question, it seems Metatransactions is the main cause of the work and limitations here, are there other alternatives to Metatransactions that could make this easier? Such as using account abstraction with EIP-3074 or the incoming EIP-4337?

It's not clear to me these help. Fundamentally, in all cases, we need to get the user to sign something intelligible with their private key and those EIPs seem to be concerned with what you use that signature for? EIP712 remains the way to (natively) have an intelligible message, and it is the generic, recursive encoding that it specifies that is the problem behind an all-encompassing generic implementation. For EIP-4337, at least, there seems to be some suggestion that you would use a Metamask Snap to improve the user experience - which is already an option.

We provide a user friendly way of comparing the blob in the frontend, e.g. providing the transaction details, encoding and expected hash to compare it, within the transaction list or just the transaction details provide links to resources where they can do it themselves.

I guess this helps from a user-experience perspective, but I'm not convinced it helps from a meaningful one. If I don't trust the blob being given to me by Colony, should I trust the tool that Colony have given me to verify it? EIP712 being native and interpreted by Metamask or whatever other wallet software I think is a real benefit, as it's much less likely Metamask and Colony are colluding against me.

(Of course, regardless of what Metamask says, it's possible the implementation on the contracts ignores what I've passed as the parameters and does something different... at that point, what I should be doing is simulating the transaction on Tenderly, or similar, to see what it actually does, and the nice EIP712 message that I'm signing isn't hugely meaningful.)

We go with the approach as you have described but only for specific transactions, such as for pivotal user onboarding transactions e.g. creating a colony, making a payment, finalizing a motion.

This makes sense - this was the same logic that led to us implementing SIWE for logging in. For what it's worth, I would probably be in favour of doing this for all functions on the contracts, and then being implemented piecewise on the frontend as functions either get implemented or we see demand where they're being used.

One other thing I've realised: The implementation of
EIP2612 that we used for the token we are currently deploying uses the chainId in the domain separator - that means that to send metatransactions to the tokens we've been deploying, users will have to switch chains. If we decide on our non-standard use of salt being okay in our implementation of EIP712, this can be changed, but tokens already deployed cannot be upgraded.

@arrenv
Copy link
Member

arrenv commented Oct 4, 2024

@area thank you for the additional info. I shall put together a list of transactions I think are critical user experience points, even if you end up doing them all. Are there any you think should be essential?

Regarding colony deployed tokens, it seems you are yet to see any issues with using the salt approach for chainId, although diverting from the standard does seem more risky then not and I wonder if it is something we should disclose for users who create a new token on colony if we do do it. Even though it works Metamask, is there a way to determine future proof compatibility, such as with other wallets? Is there a risk of tokens not being able to be bridged?

@area
Copy link
Member Author

area commented Oct 4, 2024

There aren't really any transactions that I'd class as essential, but I would be drawn to the ones that, in the lifecycle of a particular feature, are used most often. So, for example, in VotingReputation, I would say that the staking/voting/reveal transactions would be the most important, with creation and finalisation secondary; the first group occurs multiple times in the lifecycle of a motion, whereas the second group only occurs once per motion.

I guess the possible downside with a partial implementation is that the scary unknowable blobs might be even more scary, if users are used to friendlier messages elsewhere.

While using the chainId as the salt goes against the default (typical?) implementations of EIP712, that's more on the contracts side when it comes to validating EIP712 signatures. What I'm suggesting is still 100% in-spec, so I would not anticipate any incompatibility in the future from any wallets, Metamask or no (as it would mean, I believe, they were not actually EIP712 compatible). I would not expect it to affect the bridge-ability of a token, either (do you have a mechanism in mind for how it might cause issues? I'm not sure I see how they would be related at all).

@area
Copy link
Member Author

area commented Oct 4, 2024

Multicall

I don't, at this point (though it is getting late on a Friday), believe it's possible to do the EIP712 signing in conjunction with multicall. Or more precisely, it is, but it is not possible to keep the elements human readable (it would appear as Multicall with the arguments being the unintelligible blobs that we are used to seeing). Or, alternatively, it is, but every possible combination of desired multicalls would require a function written on the contracts, somewhat defeating the point.

One possibility would be that each component of a multicall could be signed separately using EIP712, bundled up together and broadcast as one transaction by the broadcaster... at least then there isn't a delay between signing each 'transaction'?

@area
Copy link
Member Author

area commented Oct 8, 2024

An initial exploration of what the work would look like is on this branch

@area
Copy link
Member Author

area commented Oct 11, 2024

Given the incompatibility with multicall, and the fact that EIP712 isn't friendly enough, the current thinking is that the frontend will provide users with instructions to verify the hash they're being presented with is correct, if they want to do so. As an example, the instructions for makePaymentFundedFromDomain would be:

iface = new Interface(["function makePaymentFundedFromDomain(uint256,uint256,uint256,uint256,address[],address[],uint256[],uint256,uint256)"])
txdata = iface.encodeFunctionData("makePaymentFundedFromDomain", [1, "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", 1, "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", ["0x27ff0c145e191c22c75cd123c679c3e1f58a4469"],["0xd5eb87c756fcb12df07a100f0326c4ae17fd1dde"],["10"],1,6])

// The variables here are: User Nonce // Address tx is being sent to // Chain Id // Transaction data
ethers.solidityPackedKeccak256(["uint256", "address", "uint256", "bytes"], [0, "0xBaE2cc298449726064F39DED7eA84E51742130d5", "265669100", txdata])

// Should give 0xcb0ef8c45d0d07f70dcb3722cfcc631c3b15ec67e074da06e2ff543c4c28d571

These have been written such that they can be copy and pasted in to the ethers playground i.e. a web-accessible, independent and hopefully trusted tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@area @arrenv and others