Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

Borrow: Signature and chain have to match #128

Closed brozorec closed 1 year ago

brozorec commented 1 year ago

For signatures, we use signer._signTypedData(domain, types, value) and thus the messages users are signing are human-readable. However, we figured out that when a user signs a permit for a router on another chain, MM should be switched to that chain.

Option 1 We keep using signer._signTypedData(domain, types, value) and before signing the user has to be prompted to switch the network for the chain where the vault lives: vault.chainId

Drawbacks:

Option 2 Make the user sign a digest that's a non-human readable string and so avoiding the need to switch on the destination chain only for the signature.

Drawback:

0xdcota commented 1 year ago

@doliG @brozorec can you share a link to where you are currently implementing the signatures. Is in the SDK or directly on the front-end?

0xdcota commented 1 year ago

For Option 1 I am probably interested on how the chainId is obtained.

const domain = {
      // Dapp-defined parameter: in this test it is set-up in {MockToken.sol}
      name: await token.name(),
      // Dapp-defined parameter: in this test it is set-up in {MockToken.sol}
      version: '1',
      // obtained from evm opcode CHAINID for each EVM. Refer to https://chainlist.org/ and/or https://eips.ethereum.org/EIPS/eip-155
      chainId: (await ethers.provider.getNetwork()).chainId,
      // contract for which this 'domain' object belongs.
      verifyingContract: token.address
    };
brozorec commented 1 year ago

If the user is acting from chain A and the tx is going to settle on chain B because the vault where we want to open the position is there, then chainId is this of chain B. The verification of the signature is going to happen on chain B. @DaigaroCota

0xdcota commented 1 year ago

If the user is acting from chain A and the tx is going to settle on chain B because the vault where we want to open the position is there, then chainId is this of chain B. The verification of the signature is going to happen on chain B. @DaigaroCota

So you know chain B, from user input? I ask because I wonder how metamask-app knows it has to change network.

brozorec commented 1 year ago

This commit dbcc59b471c3f261da078a37dc6edd55c7f5b787 should fix the issue. @doliG can you plz test and let us know if it works?

brozorec commented 1 year ago

This commit dbcc59b should fix the issue. @doliG can you plz test and let us know if it works?

No, sorry for the premature good news, I did more tests and actually, it doesn't work :(

doliG commented 1 year ago

Hey @brozorec @DaigaroCota , thanks for this ticket.

I'm guessing from the discussion above that we'll go with the not so good UX (switch chain to sign then switch back to borrow) ?

Btw @brozorec I'm not sure as a user I can call what we currently have as something "readable" 🙃 Like you need to hacve a good understanding of how fuji works, master eth scan to check addreses and so on 🤔

brozorec commented 1 year ago

haha, good point @doliG. I was also thinking along the same lines: in both cases, it's difficult to understand for a regular user, so better if we provide them with guidance on how to "decrypt" it.

@DaigaroCota what if we move the chainId from the domain to the content? I'm aware that this may require our own implementation that's different from the standard.

doliG commented 1 year ago

@brozorec @DaigaroCota any updates on this ?

brozorec commented 1 year ago

@brozorec @DaigaroCota any updates on this ?

it should be fixed with https://github.com/Fujicracy/fuji-v2/commit/d85dcace7fcb869d4ee7b9299663d9d5e0a86546 can you confirm it's the case plz?

doliG commented 1 year ago

@brozorec it's not, got this error when trying cross chain borrowing:

doliG commented 1 year ago

@brozorec any update on this ticket ? 🤔

brozorec commented 1 year ago

@brozorec any update on this ticket ? thinking

this is fixed

doliG commented 1 year ago

That what I was thinking. I close this as won't do bc the fix is in the sdk / contracts.