Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

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

Open
Ivshti opened this issue Dec 25, 2021 · 2 comments
Labels
app:CowSwap CowSwap app Bug Something isn't working Medium Severity indicator. It causes some undesirable behavior, but the system is still functional Wallets Wallet related

Comments

@Ivshti
Copy link

Ivshti commented Dec 25, 2021

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

@elena-zh elena-zh added app:CowSwap CowSwap app Medium Severity indicator. It causes some undesirable behavior, but the system is still functional Bug Something isn't working Wallets Wallet related labels Dec 27, 2021
@elena-zh elena-zh changed the title Order signing with SC wallets uses pre-approvals instead of EIP1271 [Ambire Wallet] Order signing with SC wallets uses pre-approvals instead of EIP1271 Dec 27, 2021
@fleupold
Copy link
Contributor

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
Copy link
Author

Ivshti commented Jan 10, 2022

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:CowSwap CowSwap app Bug Something isn't working Medium Severity indicator. It causes some undesirable behavior, but the system is still functional Wallets Wallet related
Projects
None yet
Development

No branches or pull requests

3 participants