Log signed messages in state logs

[danfinlay]

We should be storing a history of signatures, especially as we move to a more counterfactual/metaTx world, and more txs are replaced by signatures.

Especially in the wake of the OpenSea/Wyvern phishing campaign, having good forensics of past signatures would be extremely useful.

Acceptance criteria:

• Log all signed messages (signTypedData, signTypedData_v[x], personal_sign, eth_sign)
• Log the origin that proposed the messages
• Log when the message is signed
• Results should be available in state logs

Stretch goals

• Show these signatures in the state-log-explorer

[ryanml]

Related UI issue: https://github.com/MetaMask/metamask-extension/issues/11178

[bschorchit]

Pasting here some discussion that happened on Slack over this so it's not lost - full thread is here

[Brad] It seems like the right way to do this is to treat signatures as true transactions, especially given that the description is "more txs will be replaced by signatures" which we currently treat as temporary only to live i n state for as long as it takes the user to sign/reject the signature. IF we do that we have to modify code in a few different places... which would make this more of an epic just to get the logs. If we want to prioritize logs WITHOUT the correct and more proper fix, to avoid larger work upfront, then we will want to add these to a different state key. Thumbs up/Thumbs down on doing part 1 of this work? we currently have signatures of all types managed by a message manager controller/code structure that we largely do not want to extend. Moving signatures to a new controller that uses Erik Mark's Approval system (which was intended from the start I believe). So basic idea: Move signature requests to use approval system, use a shared (new) controller for those signature requests, the state for that system for now is just logging the signatures...later we can figure out how to do the UI changes necessary to show them in the UI.

[Taylor] A new controller seems to me like a good solution especially if we set things up in a way that this new controller can be utilized more in the future. I don't have background on the approval spec but I'm assuming the idea is that it would be used eventually for handling all confirmations - both transactions and msg signatures? Another option (that may or may not be better, Or may be essentially the same, don't know) is creating a “receipts” or “logs” controller that is specifically for the post-state of anything. It's set up to be as robust and flexible as possible given that we don't know what future “receipt” information may include. But even tho it's flexible, it only has one task: take whatever information is sent to it, log it to state logs, and decide if or where to go from there. With this, the idea is to decouple the action and state needed for the user action and UI from the storing of information for the purposes of being recalled later. If it's flexible enough in accepting what it receives, this pattern could be useful down the road to ensure that any action is, at the very least, logged somewhere, even if the PM or engineers aren't specifically thinking about that element of a feature. It also means that if in the future we want to change where exactly the state logs are stored or something of that nature, there's a single place to do so from and you don't have to go thru every feature. For now it could just be msg signatures -> receipt -> save to logs. In the future it could be msg and txns and even things like “date time: installed XYZ snap” go into receipts, all are saved to logs, and some are then passed to the txn history controller to be displayed, others go to a installed/uninstalled Snap view, etc.

[Mark] We talked about this years back but haven't managed to find time to do the work. On the semantics it wasn't so much treating them like transactions, but as being on the same level as transactions. That's why our Activity log is called an Activity log rather than a Transaction history. Signatures are just as important as transactions to have a record of. But I agree, that's a long term effort that involves a lot of work that isn't necessary for just forensics. Logging signatures somewhere else for now makes sense.

[Mark] This sounds like a great idea! And we could even migrate our existing transaction logs to here. Not the entries we show to the user, but the logs we have within each transaction group that are only used for diagnostic purposes. Those have already caused issues with state bloat.

[danjm]

@Gudahtt Regarding the above chat that Barbara copied, what are you referring to when you say "This sounds like a great idea!"? Was it Brad's suggestion to "Move signature requests to use approval system, use a shared (new) controller for those signature requests, the state for that system for now is just logging the signatures...later we can figure out how to do the UI changes necessary to show them in the UI."?

And is that controller the "somewhere else" you were referring to when you said "Logging signatures somewhere else for now makes sense."

@brad-decker It's a little unclear to me whether the "basic idea" you describe was meant to be the simpler the way to "avoid the larger work" that you referenced when saying "If we want to prioritize logs WITHOUT the correct and more proper fix, to avoid larger work upfront, then we will want to add these to a different state key." Or is the simpler approach to just getting logs for forensics not described in the quoted comments from Barbara?

[bschorchit]

Here is the link to Mark's original message, @danjm. You can check what he was quote replying to - I did an incomplete job on copying and pasting it here 😅

[brad-decker]

@danjm I think the proper approach for short term, without getting into the approval system usage for signing requests, is to do what @tayvano suggested of creating a logging controller that just stores logs. A controller of this spec could de lightweight and doesn't impair our ability to do the approval refactor that i mentioned in a later PR

[bschorchit]

Moving this to the "Ready for refinement" column. We should proceed with approach outlined by Brad above.

[ElvirCe]

Moving this to "Pending ConsenSys" as Dan Miller advised that he needs to work on the technical solution in more detail with the team. Dan will organize calls to identify all technical requirements.

[bschorchit]

For visibility: I'm un-assigning this from PS team. A confirmations team is being formed and they will to work through the technical solution and implementation early next year.

Update: work was already in progress here https://github.com/MetaMask/core/pull/1089 and controller will be used for other use cases besides signatures so this was not transferred to confirmations team.

[holantonela]

With https://github.com/MetaMask/metamask-extension/issues/17587 closed, are we done here @kevinghim?

[bschorchit]

With https://github.com/MetaMask/metamask-extension/issues/17587 closed, are we done here @kevinghim?

Not at all, @holantonela . We're waiting for https://github.com/MetaMask/metamask-extension/issues/17751 to proceed with this issue

[brad-decker]

I don't think this work progresses anything close to #11178 that was previously mentioned as being related. This is adding persistent logs to memory for the purposes of auditing, but it isn't adding a way to retrieve the data for display in the activity list. When a signature request is resolved it is removed from the state entirely, or at least was when the controller only existed in the extension. That still will be the case the only difference being that the logging of the message details will persist for future support requests.

There should be another epic, owned by a different team, for the improvement to the activity list that treats signature requests as full fledged transactions/activity. Currently the only way to "see" the signature request in the activity list is to have the full screen UI open and then trigger a signature request on a dapp in another tab. If you tab back to your full screen MetaMask you'll see the pending signature request transaction but as soon as its signed its removed from state.

[tayvano]

Adjacent / related pull request in core: https://github.com/MetaMask/core/pull/1089/

[bschorchit]

This will be worked on by confirmations system team during Q3.

[rayeaster]

Agree with @danfinlay that it would benefit for MetaMask users if signed signature history could be retrieved easily in UI. Sometimes, user might accidentally sign some messages in some site without fully aware of what was signed. By providing the history for signed signature (with info like site, message, etc,), it would help to clarify concerns around if the signature is malicious or good

[guilyx]

Any ETA for this ?