safeDeposit and addNewVaultsByPath methods on switchboard are not entirely panic free about flow-ft

[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 onflow/cadence#2264

With capability controllers, I believe the new borrow() method from the CapCon won't panic at all onflow/flow#798 (comment)

[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: onflow/cadence#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