search

onflow / linked-accounts

The Unlicense
11 stars 3 forks source link

Implement NFT & MDV standards + FLIP#72 feedback #10

Closed sisyphusSmiling closed 1 year ago

sisyphusSmiling commented 1 year ago

Closes: #3 #5 #6 #8 #14 #17

Description

ℹ️ This PR is a second iteration to the AuthAccount Capability Management FLIP#72 based on feedback received. That FLIP will soon be updated to reflect the changes made in this PR

The biggest change in this PR is the implementation of the NonFungibleToken, MetadataViews and ViewResolver contract standards as well as renaming of the contract and its defined resources.

By implementing the NFT & Metadata standards, implementers will be able to easily resolve metadata about linked accounts - an important factor to enable administrative type overviews of their linked accounts.

Interfaces & mental models are largely aligned with current ecosystem standards; however, due to concerns around phishing attack vectors, deposit() is not publicly accessible. One can imagine an attacker might create an account they control and wrap its AuthAccount Capability in a LinkedAccounts.NFT, assigning it the same LinkedAccountMetadataViews.AccountInfo as a victim's existing account. The attacker then transfers the NFT to the target's LinkedAccounts.Collection. The target could easily mistake the compromised account for their existing trusted account and transfer assets to the attacker controlled account, allowing the attacker to withdraw the transferred assets. While there are a number of ways this vector could be mitigated (wallet notification on deposit, verification of dApp created accounts, etc.), limiting public deposits during the introduction of this feature set seems the most appropriate at the moment.

You'll also see changes to contract events, conforming with NFT contract standards as well as others that can be leveraged by wallet providers to notify users of newly linked accounts as well as provide off-chain audit logs for account linking events. These serve as data points additional to flow.AccountLinked(address: Address, path: PrivatePath) (emitted whenever AuthAccount.linkAccount() is called).

Another note is the access of AuthAccount Capabilities as well as LinkedAccounts.Handler.grantedCapabilities. Access to any encapsulated Capabilities is mediated by reference. While this has been discussed as an anti-pattern in Capability-based security, the decision was ultimately rooted in the limitations of Capabilities as they exist in Cadence today. The crux of problem is auditability & revocation of the highly security sensitive AuthAccount Capability. For instance, let's say I've been given an AuthAccount Capability for some app account - call it 0xGAME linked at 0xGAME/private/MainAccountLink. If someone gets that Capability from within a malicious transaction I have no way to

  1. determine that the Capability was retrieved
  2. distinguish my Capability from the one they copied

And further, I'm now in a pickle...if I want to revoke their Capability, I also need to revoke my own by unlinking the Capability entirely. I may prefer this. Due to this, we return a reference to the AuthAccount Capability instead of the Capability itself, at least until Capability Controllers are live.

Due to the current opaqueness around Capabilities and lack of granular revocation, it's recommended that we prevent direct of AuthAccount Capabilities and continue to lean on access by reference until auditability & revocation of Capabilities is enhanced with CapCons.

Another significant change is removal of account creation methods and the ChildAccountCreator resource. This is due to concerns around the scalability of the resource, as well as upcoming changes to the Cadence account linking API, desire to give builders greater flexibility in account creation & custodial patterns, and desire to maintain a more defined separation of concerns. Ultimately, this contract is about linking existing accounts, and the decision has been made to remove prescriptive account creation modalities from this contract.

For contributor use:

sisyphusSmiling commented 1 year ago

Thanks for the review @joshuahannan! Tests are definitely needed and will be the focus next week after solidifying the final approach.

On that note, any input on how to handle the private Collection.deposit() method in light of the MetadataViews.NFTCollectionData pre-conditions enforcing certain composite restricted types would be helpful! A couple ideas:

joshuahannan commented 1 year ago

We're in the process of removing the restriction in the metadata view, so that is possible if you can wait to deploy until after we've upgraded, maybe next week or the week after. Otherwise, probably will need to panic in deposit

austinkline commented 1 year ago

Thanks for the review @joshuahannan! Tests are definitely needed and will be the focus next week after solidifying the final approach.

On that note, any input on how to handle the private Collection.deposit() method in light of the MetadataViews.NFTCollectionData pre-conditions enforcing certain composite restricted types would be helpful! A couple ideas:

  • Remove the pre-condition or remove the Receiver & CollectionPublic composite restriction in MetadataViews.NFTCollectionData.publicLinkedType.
  • Expose the deposit() as a public method in this standard, but panic on deposit.

I'll think about this tonight and provide a suggestion, I don't think we should remove the metadata view restriction since it serves other purposes and removing it doesn't really ensure no one will link things publicly. Happy to chat offline if that's quicker as well, feel free to reach out!

austinkline commented 1 year ago

Here is a first pass at the phishing issue @sisyphusSmiling https://github.com/onflow/linked-accounts/pull/16

Not sure if it's what folks on the flow side would like, might need some work as well but it is maybe a good place to start the conversation

sisyphusSmiling commented 1 year ago

Update: Since latest discussion on this PR, hybrid custody efforts have been taken up in this repo. The delay in this PR has been a result of working out a path forward on key issues, broadly revolving around making the burden of implementation as light and ergonomic as possible for application developers.

This PR then is simply to get the repo to parity with the iteration used for the Walletless Arcade demo which was an experimental POC to showcase Walletless Onboarding and Hybrid Custody concepts and prototype implementations.