Swaps on Sushiswap with Sushiguard are failing

[SJQuality]

Build Info/PR/Commit:

https://github.com/tallycash/extension/tree/polygon-qa-3

Related Ticket:*

Reproduction Steps:

1. Be on Ethereum chain
2. Swap

Expected Result:

Successful

Actual Result:

Odd characters in Signature, failed.

https://www.awesomescreenshot.com/video/9857632?key=cad66cc8ea1712cf292a368a0b3c1a1b

[0xDaedalus]

Works fine if you disable Sushiguard:

I think we'll see this kind of stuff more-and-more as AMMs adopt steps to mitigate MEV - but dont know if its worth the lift for now (I predict a medium - high lift judging from docs on https://docs.openmev.org/)

cc @mr-michael

[jagodarybacka]

Odd characters in Signature, failed.

Addressed in #1826

Signing is still not working but as commented above - it's deeper issue

[Shadowfiend]

Went deep on this one. The issue here is that Sushi Guard expects a non-EIP-191 signature. Sushi Guard constructs a transaction, hashes it, and requests a raw signature of the transaction hash from the wallet. Metamask implements this despite concerns around the security of allowing arbitrary signing (see https://github.com/MetaMask/metamask-extension/issues/2215#issuecomment-555175590 for more) but presents a big warning about the dangers thereof.

All non-typed-data signing paths in Tally Ho right now use simple EIP-191 signing, which prefixes any data, including data that is hash-shaped, with \x19Ethereum Signed Message:\n followed by the length of the data, then hashes that string, then signs that hash. This includes calls to eth_sign and personal_sign in the providers.

Notably, non-EIP-191 signing is not available for hardware wallets, so if we implemented this path it would only work for keyring signing.

We have a few paths we can take here:

• We can add support for eth_signTransaction, which would allow the above scenario to work (including for hardware wallets) while still allowing us to present information to the user about what the transaction they're signing will actually do (ironically, Metamask does not support this due to security concerns). The issue here is it would require finding dApps that do this kind of signing and updating them or pushing that they be updated to use eth_signTransaction when available. There may also be apps that use unprefixed signing to sign data other than a transaction hash, which could not be updated to use eth_signTransaction.
• We can add support for unprefixed signing for keyring accounts, and update the UI to reflect the danger of signing this kind of data.
• We can deny support for unprefixed signing. It's not entirely clear if we can simply fail eth_sign requests, or if there are dApps that use eth_sign but expect prefixed signatures (because eth_sign has a long and strange support history).

Decision… TBD.

[0xDaedalus]

Not a priority for the next 12 months