search

gnosis / cowswap

🐮 CowSwap: First Gnosis Protocol v2 UI
https://gnosis.io
GNU General Public License v3.0
113 stars 55 forks source link

[Ambire Wallet] Order signing with SC wallets uses pre-approvals instead of EIP1271 #2044

Open Ivshti opened 2 years ago

Ivshti commented 2 years ago

Description It appears that signing orders with SC wallets uses sig mode 4, pre-signing an order (https://docs.cowswap.exchange/smart-contracts/settlement-contract/signature-schemes) instead of sig mode 3 (EIP 1271 signature)

This issue happens with Ambire Wallet

How to Reproduce Steps to reproduce the behavior:

  1. Go to wallet.ambire.com, create an account
  2. Connect via WalletConnect in CowSwap using connect -> WalletConnect -> Desktop -> Ambire
  3. Try to make a trade on CowSwap
  4. Eventually, after the ERC20 approval, you end up with a transaction to 0x9008d19f58aabd9ed0d60971565aa8510560ab41 that emits PreSignature log, rather than a signing request

Expected behavior CowSwap should be using personal_sign to request an EIP1271 signature from the wallet, to avoid extra gas costs

Screenshots Screenshot 2021-12-25 at 19-15-48 Ethereum Transaction Hash (Txhash) Details Etherscan Screenshot 2021-12-25 at 19-06-00 CowSwap Meta DEX aggregator

(note: wallet addr was deleted via DevTools in the second screenshot for privacy)

Additional context Ambire Wallet is a smrat contract wallet that supports EIP1271 as evident here https://github.com/AmbireTech/wallet/blob/main/contracts/Identity.sol#L152. This has been verified with multiple dApps such as Opensea

fleupold commented 2 years ago

Hi @Ivshti, thanks for creating the issue. It's correct that the CowSwap smart contract supports EIP1271 and it would be a better UX to use this order type in smart contract wallets that support it.

Currently the limiting factor is our backend (https://github.com/gnosis/gp-v2-services), which only supports ethsign, EIP 712 and PreSign orders.

In order to add EIP1271 to the backend one would have to follow a similar implementation path as we did for PreSign

We haven't prioritised this feature yet, since we didn't see that much demand for this order type (therefore it's good that you raised it). Of course, we would be happy to accept code contributions.

Ivshti commented 2 years ago

thanks @fleupold, we're adding this to our pipeline, we should be able to get it done by the end of the month!