safeDeposit and addNewVaultsByPath methods on switchboard are not entirely panic free

[alilloig]

Problem

safeDeposit(from: @FungibleToken.Vault): @FungibleToken.Vault? and addNewVaultsByPath(paths: [PublicPath], address: Address) could panic if the stored object pointed by the capability is changed to a new one that does not implement {FungibleToken.Receiver} . See https://github.com/onflow/nft-storefront/pull/79#discussion_r1073803180

Steps to Reproduce

Link a ft receiver to the switchboard and then remove the vault pointed by the receiver, storing on the same path any other resource than a {FungibleToken.Receiver}, then attempt to safeDeposit. Passing a path to addNewVaultsByPath that points to a non {FungibleToken.Receiver} resource.

Acceptance Criteria

Is this edge case important enough to update the contract? The solution will be to check on the capabilities before attempting to borrow them, how can check panic?

[alilloig]

@bluesign what do you think about this? I may be more worried about safeDeposit since someone could try to screw on purpose some dapp transactions that is relying on the method to ever panic. I'm probably more ok with addNewVaultsByPath since if we change the method to just skip instead of panic you will need to check on a script latter if all your vaults were actually added, so I like it how it is.

[satyamakgec]

@bluesign what do you think about this? I may be more worried about safeDeposit since someone could try to screw on purpose some dapp transactions that is relying on the method to ever panic. I'm probably more ok with addNewVaultsByPath since if we change the method to just skip instead of panic you will need to check on a script latter if all your vaults were actually added, so I like it how it is.

I do want to understand the downside of this bug, If let's say the path is not linked anymore then what happens when you borrow the capability? It will panic? or just not borrow it and returns nil?

[alilloig]

If the capability has been unlinked borrow will just return nil, but if instead, without unlinking, you load the resource that is stored under the storage path pointed by the capability, and then store there some other resource that do not implement the type of the capability, the borrow method will panic. This could be skipped by calling check on the capability first, but there's also ways of making check panic https://github.com/onflow/cadence/issues/2264

With capability controllers, I believe the new borrow() method from the CapCon won't panic at all https://github.com/onflow/flow/pull/798#issuecomment-1411241981

[bluesign]

btw I think this panicking behaviour is a feature, otherwise returning nil can have 2 meanings: it is empty or different type stored.

ref: https://github.com/onflow/cadence/issues/1247

[satyamakgec]

Yeah I also think panic is a feature here, And let's say panic happens, then isn't it good that the transaction failed and not jeopardise funds? Maybe you want that transaction would never fail, as some attacker can abuse this feature and never let the transaction succeed.

[alilloig]

Probably the best change here will be to clearly explain in which situations this methods could panic so developers using them are well aware of it.

A deposit method that will ever panic would be useful for paying royalties, that was my original thought about this