One of the last TODO comments in HybridCustody. We need to handle the case where a child tries to publish to a parent multiple times.

Discussion can be found here:
https://discord.com/channels/613813861610684416/1087374662100602920/1120753477338009660

Summary:

1. We'll fail to publish the duplicate
2. Add a method for the child to revoke access
3. If possible, child revoking a parent should clear the manager. If it can't, the parent needs to do that cleanup itself.

The issue of proxy account creation maintaining the capability once republished gets resolved with capcons, I think we can tackle that prior to launching but after audit feedback looks good

Description

pub will be removed in favor of access(all) in preparation for entitlements. As such, we should ensure that access modifiers are updated before mainnet deployment.

Suggested solution

Replace instances of pub with access(all) across the whole of this codebase.

Description

In contracts/HybridCustody.cdc:672, the removeParent function unlinks the public path for the proxy account identifier. This is unnecessary because public paths are not linked during the creation of the proxy account resource.

Recommendation

We recommend removing the unneeded line.

CLI scaffolds make it easy for developers to quickly start projects locally, and it would be helpful to make these contracts accessible as possible through the scaffold tooling.

This should be pretty straightforward. I believe we just need to submit a PR to flow-cli adding the base repo to this file along with some relevant info. Though we may want to create a separate scaffold repo.

Description

In contracts/HybridCustody.cdc:223, the filter variable acts as a default filter value passed to any newly added child account. Since the manager resource owner cannot modify this, any new filter the manager intends to add requires calling the setManagerCapabilityFilter() again. This can easily get complicated when the number of child accounts increases.

Recommendation

We recommend allowing the manager to modify the default capability filter.

Description

In contracts/CapabilityFactory.cdc:17, the function addFactory() doesn’t check if the type that is being added already exists or not. If the type to be added already exists, it may be overwritten by mistake.

Recommendation

We recommend implementing a updateFactory() function which allows updating existing factory types and modifying the addFactory() function only to accept new types. Alternatively, we recommend documenting the intended behavior of the function to educate developers.

Description

This was noted as a TODO comment in the contract, and was also highlighted in the recent audit report.

In contracts/HybridCustody.cdc:593, no validation ensures the publishToParent function is not called towards the same parent address more than once. If the function was called twice for the same parent address, the old ProxyAccount resource will be removed, as seen in line 621. This is inefficient because the child account should call the removeParent function to overwrite an existing parent.

Recommendation

We recommend adding a precondition check that ensures self.parents[parentAddress] == nil before publishing to parent.

Context

We need two sample transactions for this:

1. ProxyAccount redemption by a manager setting the display metadata view as an additional step
2. Creating a child account resource and setting the display metadata view

Related: #16

It was noted in the audit report that it was not possible to update ProxyAccount.display as the parent account since ProxyAccount.setDisplay() is access(contract). However, this functionality is enabled by Manager.setChildDisplay().

This misconception highlights the need for better documentation on this fact. Any documentation should cover the displays for both ChildAccount and ProxyAccount, who can change each, when in the linking process each should be set, which resource to rely on as source of truth, and how to resolve this.

Other topics to note might include warning about phishing vectors, and how to add additional view types via data buckets.

Description

In contracts/HybridCustody.cdc:429-431, the setDisplay function in the proxy account resource is not callable by any functions. Consequently, the parent account cannot set the proxy account’s display field, as indicated in line 404.
We classify this issue as major because it affects the correct functioning of the system.

Recommendation

We recommend allowing the parent account to call the function.

Description

In several instances of the codebase, many unimplemented functionalities are marked as TODO. This decreases the readability of the codebase.

Recommendation

We recommend implementing the required functionalities or resolving them for better code practices.

• Now that the general layout of the HybridCustody contract is done, we need to make sure that all discoverability requirements are met for each of our main resources. A good place to start is the current flow doc which shows an old implementation of Child Accounts. We should make sure we can replace each of the main examples found here

Discoverability Checklist TODO:

• #31

WIP / In Review

• #45

DONE

• Revoking Secondary Access on a Child Account
  • Maybe not relevant to latest standards, parent can't revoke
• Account Creation
• Publish & Claim
• #23
• #38
• #26
• #28
• #24
• #29
• #27
• #25
• #43

Description

In several instances of the codebase, capabilities are not validated to be borrowable before storing them. The following code locations should have the capability validated:

• addCapability() in contracts/CapabilityProxy.cdc:87-94.
• Initialization phase in contracts/HybridCustody.cdc:354 where the filter is not nil.
• setManagerCapabilityFilter function in contracts/HybridCustody.cdc:418 where the managerCapabilityFilter is not nil.
  Consequently, the capabilities might fail to borrow the underlying resource reference when used, which is inefficient.

Recommendation

We recommend validating the capabilities mentioned above.

Current Behavior

CapabilityProxy.Proxy.getAllPrivate() currently returns public Capabilities

pub fun getAllPrivate(): [Capability] {
    return self.publicCapabilities.values
}

Should be

pub fun getAllPrivate(): [Capability] {
    return self.privateCapabilities.values
}

Expected Behavior

As method names suggests, should return contained private Capabilities from Proxy

Description

In contracts/HybridCustody.cdc:707, the giveOwnership() function sets the acctOwner to the recipient to indicate they own this child account. However, there is a possibility that the recipient does not claim the published capability from the child's account. Consequently, the getOwner() function in line 690 would still show the account owner is the recipient, which is incorrect.

Recommendation

We recommend only setting the account owner to the recipient once they have called the addOwnedAccount() function.

Description

In contracts/CapabilityFilter.cdc:34-37 and lines 72-75, the removeType() function removes the capability from the dictionary without checking its existence. This is problematic because the FilterUpdated event would be emitted accordingly to indicate the capability is inactive, which is incorrect. After all, the capability was never added before.

Recommendation

We recommend only emitting the event if the capability is found inside the dictionary, similar to the implementation in contracts/HybridCustody.cdc:310-317.

Given a Hybrid Custody resource, we'll want to determine metadata about the associated accounts. For example, a wallet will want to display to a user the application associated with a child account and perhaps even the access the user has on that account.

The details of the sorts of views we introduce at the outset are up for discussion, this issue just serves as a place of discussion on the topic.

As I understand it, ProxyAccount.managerCapabilityFilter is supposed to act as an additional filter on Capability retrieval; however, it's only ever set in the current contract.

To fulfill it's intended function, it should be checked in any method that retrieves a Capability. Those are:

• getCapability()
• getPrivateCapFromProxy()
• getPublicCapFromProxy()
• getPublicCapability()

We haven't taken the time to dive into what events we need to properly allow indexers to know what accounts are managed by a certain address. We should break down what questions an indexer needs to be able to answer through events, and make sure that we are handling them in the contract that we end up using for Hybrid Custody.

This will play into discoverability as well, which some folks in our discussions have brought up.

Hybrid Custody accounts are going to come in many flavors with different rules and restrictions. In order to not take a purview on which ones are valid and which ones are not, we should consider defining an interface to sit on-top of an AuthAccount which can be used to control certain aspects of the AuthAccount itself. This suggestion comes from Dete in our latest community call. You can find context about that and the ongoing discussion about hybrid custody here

Context

Related: #25 #43

Following queries in PR #39, we'll additionally want to know not only which NFTs are owned in associated accounts, but those that are accessible from a HybridCustody.Manager. This will require knowledge of those Capabilities accessible from a given ProxyAccount (#43 )

Currently, ManagerPrivate.borrowOwnedAccount() should return a restricted reference to an OwnedAccount (formerly ChildAccount) that includesMetadataViews.Resolver.

pub fun borrowOwnedAccount(addr: Address): &{OwnedAccountPrivate, OwnedAccountPublic}?

should be

pub fun borrowOwnedAccount(addr: Address): &{OwnedAccountPrivate, OwnedAccountPublic, MetadataViews.Resolver}?

Docs are out of sync with implementation after audit updates (especially #97).

Docs & diagrams should be updated to reflect the new state.

Description

The readability of the project can be further improved in the following contexts in contracts/HybridCustody.cdc:

1. The variable and function names used to denote the type of account are inconsistent across the contract. Some of them can be useful when referenced within the context it is defined but results in reduced readability in general. Consider explicitly naming the account types and identifiers and keeping them consistent across the contract. For example, lines 211 and 363 use both childAccount and account to specify a child account. In this case, explicitly calling out the child's account can improve readability.
2. The seal() and removeOwned() functions do not sound as cautious as they need to be, possibly causing their impact to be undermined. Consider making their importance more explicit in addition to the comments already given in the contract.

Recommendation

We recommend applying the recommendations mentioned above.

Query the Capabilities accessible from a given ProxyAccount Capability in Manager.accounts

Script interface would probably look something like:

pub fun main(parent: Address, account: Address): [Type]

We'll want to update Hybrid Custody docs in the developer portal to reflect this contract suite

The AccountUpdated event has the same values for removal from a Manager as being published for a parent account.

// Manager.removeChild() emits
AccountUpdated(id: id, child: cap.address, parent: self.owner!.address, active: false)
// OwnedAccount.publishToParent() emits
AccountUpdated(id: self.uuid, child: self.owner!.address, parent: parentAddress, active: false)

This ambiguity can make it difficult to deduce state changes. These two actions should emit distinct event values.

createChildAccount() should be named createOwnedAccount ()

Given the fact ChildAccount and OwnedAccount both implement MetadataViews.Resolver and Capabilities for both are contained in the Manager, it would be a familiar pattern for the Manager to implement MetadataViews.ResolverCollection.

Currently, Manager implements Resolver, but AFAIK there are no plans for the resource to resolve metadata views itself. Implementing ResolverCollection would make an easier entrypoint for wallet providers and the like to resolve metadata for all of a user's child and owned accounts.

Two variations on transactions taking a user from onboarding through to Hybrid Custody with a newly created account:

1. Multi-signed by parent & client creating & configuring child account resources & capabilities and adding to parent account's Manager
2. Single-signed by client creating & configuring child account resources & capabilities and publishing for parent address

Description

Noted in the audit report for DenyListFilter,allowed(_ cap: Capability) returns true for invalid Capabilities.

In contracts/CapabilityFilter.cdc:44, the allowed function returns true when the capability cannot be borrowed. This is problematic because a malicious parent account can store invalid capabilities and use them once the underlying resource becomes borrowable. Consequently, this allows the parent account to bypass the filter restrictions created by the child account.

Suggested Fix

Recommendation
We recommend returning false to prevent the parent from retrieving invalid capabilities.

The merging PR should also contain a passing test case for the vulnerability noted in the audit report.

Related: #31

We'll want scripts to resolve MetadataViews on ChildAccount & ProxyAccount resources for a Manager's ownedAccounts & accounts respectively.

Currently, ManagerPrivate enables removal and borrowing of accounts, but doesn't enable addition of new accounts. IMO it would be more consistent to add addAccount() and addOwnedAccount() methods to the interface.

// Current interface definition
//
pub resource interface ManagerPrivate {
    pub fun borrowAccount(addr: Address): &{AccountPrivate, AccountPublic, MetadataViews.Resolver}?
    pub fun removeChild(addr: Address)
    pub fun removeOwned(addr: Address)
    pub fun borrowOwnedAccount(addr: Address): &{OwnedAccountPrivate, OwnedAccountPublic, MetadataViews.Resolver}?
    pub fun setManagerCapabilityFilter(cap: Capability<&{CapabilityFilter.Filter}>?, childAddress: Address) {
        pre {
            cap == nil || cap!.check(): "Invalid Manager Capability Filter"
        }
    }
}

• Add access(contract) methods that flow down to ChildAccount s.t. parent is removed if exists in ChildAccount.parents
• Sample txn that calls removeChild()

We need to rename a few resources, and the variables which interact with them, to be more clear about their role.

1. @ChildAccount -> @OwnedAccount. This makes it clear that the @OwnedAccount is meant to express full ownership of an account. It also has the benefit of not mixing the term child between types of access which is currently an issue
2. @ProxyAccount -> @ChildAccount. The @Manager resource calls capabilities to @ProxyAccount resources children in various ways, and the @ProxyAccount resource's interfaces are referred to as children. We should unify this
3. @CapabilityProxy -> @CapabilityDelegator. The word proxy seems to be confusing to folks, we should rename it to be more clear. Delegation is a common design pattern, which should make it more familiar than proxy

Manager.giveOwnerShip() removes the specified owned account twice:

pub fun giveOwnerShip(addr: Address, to: Address) {
    let acct = self.ownedAccounts.remove(key: addr)
        ?? panic("account not found")
    self.ownedAccounts.remove(key: acct.address)

    acct.borrow()!.giveOwnership(to: to)
}

Very minor, but I'd like to include coverage for this method and rename to giveOwnership() for consistency with the ChildAccount method.

Current Behavior

The current error message in the codebase states:

Should be:

self.ownedAccounts[cap.address] == nil: "There is already an owned account with this address"

Description

In contracts/HybridCustody.cdc:327-333, the giveOwnerShip() function calls the child account to transfer ownership to another user. However, the AccountUpdated event is not emitted to notify event listeners that there is a change in the owned account.

Recommendation

We recommend emitting the AccountUpdated event with proxy and active attributes as false.

Description

In contracts/HybridCustody.cdc:279, the getAddresses() function performs the same action as getChildAddresses(). This is inefficient because calling both functions returns the same functionality and result.

pub fun getAddresses(): [Address] {
    return self.accounts.keys
}
pub fun getChildAddresses(): [Address] {
    return self.accounts.keys
}

Recommendation

We recommend removing the unused function.

Tracker for action items resulting from this repo's audit report.

Tracking

Major

• #60
• #61
• #62

Minor

• #63
• #64
• #65

Informational

• #66
• #67
• #68
• #69
• #70
• #71
• #72
• #73
• #74

Current Behavior

Our gh action which runs our test suite doesn't detect a failure properly. If you make a PR with failing tests, the action itself will still look like it passed

Expected Behavior

A failing test should abort the test and propagate that failure up to the action

Steps To Reproduce

1. Write a test that always fails
2. Make a PR
3. GH action will pass

Environment

N/A

What are you currently working on that this is blocking?

N/A

Description

In contracts/HybridCustody.cdc:801, the display metadata view is stored in a dictionary with a hardcoded key “display”. Since the field and the functionality is already known, hardcoding the parameter can be avoided.

Recommendation

We recommend using dedicated fields for any known variables.

Discussion

This was discussed while exploring metadata views and resolution patterns for this contract. We'll want to decide if a display field should be included in ChildAccount the way it is in ProxyAccount.