[BUG] Potential incorrect owner query before ownership acceptance

[sisyphusSmiling]

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.

[sisyphusSmiling]

After thinking about this for a bit, I think there might be a larger issue here that could simply be one of expectation.

I would expect that when someone calls ChildAccount.giveOwnership(), they're granting ultimate authority over the underlying account, which is access to a ChildAccount in the construction. And in so doing, I expect that their access is either eliminated or demoted in comparison to the access they just granted. But this isn't the case.

Taking a closer look at giveOwnership():

pub fun giveOwnership(to: Address) {
    // Revoke all keys and reduce AuthAccount access to a single derived Capability path
    self.seal()
    
    let acct = self.borrowAccount()
    // Derive a path for the new owner's ChildAccount Capability
    let identifier =  HybridCustody.getOwnerIdentifier(to)
    // Link the ChildAccount Capability
    let cap = acct.link<&{OwnedAccount, ChildAccountPublic, ChildAccountPrivate, MetadataViews.Resolver}>(PrivatePath(identifier: identifier)!, target: HybridCustody.ChildStoragePath)
        ?? panic("failed to link child account capability")

    // Publish to the new owner
    acct.inbox.publish(cap, name: identifier, recipient: to)
    self.acctOwner = to
    self.relinquishedOwnership = false

    emit OwnershipGranted(id: self.uuid, child: self.acct.address, owner: to)
}

At the end of this, the caller of giveOwnership() still has the same access as the new "owner" as their Capability still exists. While this isn't necessarily a problem for an app granting ownership to a user for the first time, this could be an attack vector if users have a naïve notion of "owner".

For example, I could be the current "owner" of a ChildAccount and call giveOwnership(to: 0xALICE). Alice claims this ownership, adding the published ChildAccount Capability to her Manager.ownedAccounts, even noting that ChildAccount.acctOwner is listed as her Address. However, both she and I have the same access on the account.

Here are some ideas around how to address this issue:

1. Rename "ownership" to something that denotes a level of access, not level of authority. This makes is more apparent that the access can be shared. Some options:
   • giveOwnerAccess()
   • giveSudoAccess()
   • grantRootAccess()
2. Allow for multiple owner Addresses - acctOwners: [Address]. This makes it clear that it's possible for multiple actors to have the same level of access.
3. Disallow reassignment of owners. Once giveOwnership() is called, it can never never be called again based on assignment of acctOwner. This is likely most consistent with the original intention of the design.

[austinkline]

Thanks for the write-up @sisyphusSmiling, great catch, it looks like we're neglecting to unlink other owner capabilities when sealing a ChildAccount, we should add that.

We could certainly permit multiple owners, though I worry that could encourage a level of trust that isn't actually safe. Anyone who "owns" an account can unlink or link any other capability, meaning they could harm the other owner if they wanted to. Perhaps that's something we'd decide to live with, but at the same time, you an achieve multiple owners as the current one by publishing your own ownership capability.

I would caution against allowing an action to only take place once. With the exception of calling seal (or whatever we name it in the end), everything should be recoverable in some shape or form, otherwise developers run the risk of permanently bricking their account in ways they didn't anticipate. And besides, I could always just remake a new ChildAccount resource as the owner of the current one and get around any restrictions there

My takeaway from this is:

1. We need to iterate over all private paths and unlink any ownership capabilities when an account is sealed
2. It sounds like there's interest in expressing joint ownership which we can explore

[sisyphusSmiling]

We could certainly permit multiple owners, though I worry that could encourage a level of trust that isn't actually safe.

Initially, I agree with this. I worry though that by attempting to enforce a single owner, we'll run into the typical anti-patter of attempting to prohibit what we can't prevent. As you said, you could simply publish your own ownership Capability or create another ChildAccount resource.

I would caution against allowing an action to only take place once.

Point taken. A negative consequence of this could be janky workarounds on the part of the developers that mangle the whole thing anyway.

We need to iterate over all private paths and unlink any ownership capabilities when an account is sealed

Thinking about this, it looks like we'd need to avoid unlinking the Capabilities ProxyAccounts maintain or giveOwnership() will break all parent accounts' access. So, we'd unlink any ChildAccount Capabilities except for those at HybridCustody.ChildPrivatePath within the giveOwnership() function.

A sneaky actor could just link their owner Capability at the canonical path, but this might be remediated by CapCons. This deserves a bit more thought, but I think the general guidance should be that users shouldn't trust hybrid custody accounts with highly sensitive assets regardless of the path forward.

[austinkline]

Maybe I'm just a tad confused, here. Would we not be unlinking the following path upon giving ownership to another account?

https://github.com/onflow/hybrid-custody/blob/main/contracts/HybridCustody.cdc#L730

Thinking about this, it looks like we'd need to avoid unlinking the Capabilities ProxyAccounts maintain or giveOwnership() will break all parent accounts' access.

Correct, so we only want to remove capabilities which target the child account resource. I guess we could cycle all ProxyAccount resources and recreate their child account capabilities as well, but we might not be able to do all of that work within the constraints of compute limits in a transaction.

A sneaky actor could just link their owner Capability at the canonical path, but this might be remediated by CapCons

Very true. I think capcons solve this but we should double check. Persistent Capabilities seem to pop up for this contract quite often as an attack vector. I tend to agree with the sentiment of "this is a useful feature, but do not blindly trust".

I think the most common version of this is "Does Alice trust Application X to actually give ownership to them?" I'd say the answer to that is generally going to be yes if it's a trusted source. That assumption would absolutely break down under other circumstances, though

[sisyphusSmiling]

. Maybe I'm just a tad confused, here. Would we not be unlinking the following path upon giving ownership to another account?

You're right, we could simply unlink the Capability for the current owner if one is assigned. I was thinking about things from the perspective of a post-condition - the only ChildAccount Capabilities that should exist are those at HybridCustody. ChildPrivatePath and that derived for the new owner, which would entail unlinking anything else. That said, this approach would be overkill if we're assuming honest actors.

I guess we could cycle all ProxyAccount resources and recreate their child account capabilities as well, but we might not be able to do all of that work within the constraints of compute limits in a transaction.

Agreed

That assumption would absolutely break down under other circumstances, though

Point taken. I think the best path forward here is to simply unlink the Capability at the path derived for the current owner and address necessary trust assumptions in documentation. Does that plan sound good to you?

[austinkline]

SGTM!