Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Durin support #2478

Closed
ricmoo opened this issue Jan 2, 2022 · 7 comments
Closed

Add Durin support #2478

ricmoo opened this issue Jan 2, 2022 · 7 comments
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@ricmoo
Copy link
Member

ricmoo commented Jan 2, 2022

Add support for Durin off-chain execution.

See EIP-3668

@ricmoo ricmoo added enhancement New feature or improvement. minor-bump Planned for the next minor version bump. on-deck This Enhancement or Bug is currently being worked on. labels Jan 2, 2022
@ricmoo
Copy link
Member Author

ricmoo commented Feb 22, 2022

Anyone watching this repo/issue, I'm working on the initial security model for CCIP Read, and would love to bounce these ideas off you. :)

The concern with CCIP-read is that is allows a contract author to manipulate a caller (via normal blockchain interactions) into calling an external server, which could be controlled by the author running IP tracking software or targeting a specific URL to DDoS the service. Here is a short article outlining some of the concerns which have been found in the wild (largely for PoC).

For an example of my concern, imagine running an ERC-20 token, where the contract.symbol() method will use CCIP Read to query my server, returning it's static symbol, but also logging the IP address. Most people would not call this method, but anyone who imports my token into Uniswap will, so I will have the IP address and possibly (depending on whether the contract is connected to a Signer) may reveal the owner address too.

So, for v5.6 CCIP Read support, these are my planned default security considerations:

  • txRequest.ccipReadEnabled = false; this is by default disabled, so using provider.call will not use CCIP Read unless explicitly opted into. In v6, this will likely be enabled by default.
  • ENS resolution (via providers.resolveName) will enable ccipReadEnabled but also force the sender address to be the zero address, which is used during CCIP Read template expansion. ENS resolution is a more constrained interaction, which is usually performed by providers (so there is no sender) and is not performed automatically in most situations. Signers also squelch their address, since the default action uses this.provider.resolveName.
  • provider.disableCcipRead = false; this is by default disabled, but enabling it will prevent all CCIP Read, including ENS resolution. This is useful for applications which wish to error on the side of caution and just want to upgrade to the latest ethers. This will disable CCIP Read for ENS resolution.

Feedback? :)

@serenae-fansubs
Copy link

"enabling it will prevent all CCIP Read attempts to fail" should be "enabling it will cause all CCIP Read attempts to fail" right?

I like the idea of not enabling by default yet. And I would vote to keep it disabled until there are options in popular wallets to:

  • Enable/disable CCIP-read
  • Prompt the user before any external server requests are made, showing the URL

If it just gets enabled in v6 without any ability for users to disable it (if they're using a wallet/client that uses ethers) or at least have some transparency into what external servers are being called with what data, I think that would be a very poor security/privacy outcome for the average person.

@atropos0902
Copy link

atropos0902 commented Feb 23, 2022

@ricmoo Please don't forget I am still waiting for the updates on the block polling issue.
#2652
Waiting for the saver on v5.6 🙏

@ricmoo
Copy link
Member Author

ricmoo commented Feb 24, 2022

@serenae-fansubs Yes, you are correct. I've corrected the OP.

Using provider.resolveName is something that is used as more of an on-demand process, which is one reason I'm thinking it is safer. It can also internally remove the sender. So by default CCIP-Read will be disabled, except for provider.resolveName. There are some very cool things coming in the ENS space once CCIP-Read is out.

In v6 there is a whole plug-in system for Providers which will be used for much finer-grain support for vetting each request allowing, for example prompting the user, whitelists, blacklists, etc. as well as allows intercepting and fetching it for the user, allowing for example MetaMask to proxy all requests through their own anonymizing proxy.

@ricmoo
Copy link
Member Author

ricmoo commented Mar 4, 2022

The initial support has been added (along with test cases) in ae23bb7, if anyone has some spare time to look through it.

I still have a few things to do for v5.6, so this won't be published for a few more days.

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. minor-bump Planned for the next minor version bump. labels Mar 10, 2022
@ricmoo
Copy link
Member Author

ricmoo commented Mar 10, 2022

This has been added in v5.6.

Anyone curious, please try it out. :)

@ricmoo ricmoo closed this as completed Mar 10, 2022
@sambacha
Copy link

awesome, thanks @ricmoo

side note:

maybe add a comment that 0x9061b923 comes from ENSIP-10 ae23bb7#r68123910

would be good reference. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests

4 participants