Cadence Anti-Patterns
This is an opinionated list of issues that can be improved if they are found in Cadence code intended for production.
Avoid using authorized account references as a function parameter
Problem
A developer may choose to authenticate or perform operations for their users by using the users' account addresses.
In order to do this, they might add a parameter to a function which has an authorized account reference type (auth(...) &Account),
as an authorized account reference can only be obtained by signing a transaction.
This is problematic, as the authorized account reference allows access to some sensitive operations on the account, for example, to write to storage, which provides the opportunity for bad actors to take advantage of.
Example:
_37..._37// BAD CODE_37// DO NOT COPY_37_37// Imagine this code is in a contract that uses a `auth(Storage) &Account` parameter_37// to authenticate users to transfer NFTs_37_37// They could deploy the contract with an Ethereum-style access control list functionality_37_37access(all) fun transferNFT(id: UInt64, owner: auth(Storage) &Account) {_37 assert(owner(id) == owner.address)_37_37 transfer(id)_37}_37_37// But they could upgrade the function to have the same signature_37// so it looks like it is doing the same thing, but they could also drain a little bit_37// of FLOW from the user's vault, a totally separate piece of the account that_37// should not be accessible in this function_37// BAD_37_37access(all) fun transferNFT(id: UInt64, owner: auth(Storage) &Account) {_37 assert(owner(id) == owner.address)_37_37 transfer(id)_37_37 // Sneakily borrow a reference to the user's Flow Token Vault_37 // and withdraw a bit of FLOW_37 // BAD_37 let vaultRef = owner.borrow<&FlowToken.Vault>(/storage/flowTokenVault)!_37 let stolenTokens <- vaultRef.withdraw(amount: 0.1)_37_37 // deposit the stolen funds in the contract owners vault_37 // BAD_37 contractVault.deposit(from: <-stolenTokens)_37}_37...
Solution
Projects should find other ways to authenticate users, such as using resources and capabilities as authentication objects. They should also expect to perform most storage and linking operations within transaction bodies rather than inside contract utility functions.
There are some scenarios where using an authorized account reference (auth(...) &Account) is necessary,
such as a cold storage multi-sig,
but those cases are rare and such usage should still be avoided unless absolutely necessary.
Public functions and fields should be avoided
Problem
Be sure to keep track of access modifiers when structuring your code, and make public only what should be public. Accidentally exposed fields can be a security hole.
Solution
When writing your smart contract, look at every field and function and make sure
that require access through an entitlement (access(E)),
or use a non-public access modifier like access(self), access(contract), or access(account),
unless otherwise needed.
Capability-Typed public fields are a security hole
This is a specific case of "Public Functions And Fields Should Be Avoided", above.
Problem
The values of public fields can be copied. Capabilities are value types, so if they are used as a public field, anyone can copy it from the field and call the functions that it exposes. This almost certainly is not what you want if a capability has been stored as a field on a contract or resource in this way.
Solution
For public access to a capability, place it in an accounts public area so this expectation is explicit.
Public admin resource creation functions are unsafe
This is a specific case of "Public Functions And Fields Should Be Avoided", above.
Problem
A public function on a contract that creates a resource can be called by any account. If that resource provides access to admin functions then the creation function should not be public.
Solution
To fix this, a single instance of that resource should be created in the contract's initializer, and then a new creation function can be potentially included within the admin resource, if necessary.
Example
_36// Pseudo-code_36_36// BAD_36access(all) contract Currency {_36 access(all) resource Admin {_36 access(all) fun mintTokens()_36 }_36_36 // Anyone in the network can call this function_36 // And use the Admin resource to mint tokens_36 access(all) fun createAdmin(): @Admin {_36 return <-create Admin()_36 }_36}_36_36// This contract makes the admin creation private and in the initializer_36// so that only the one who controls the account can mint tokens_36// GOOD_36access(all) contract Currency {_36 access(all) resource Admin {_36 access(all) fun mintTokens()_36_36 // Only an admin can create new Admins_36 access(all) fun createAdmin(): @Admin {_36 return <-create Admin()_36 }_36 }_36_36 init() {_36 // Create a single admin resource_36 let firstAdmin <- create Admin()_36_36 // Store it in private account storage, so only the admin can use it_36 self.account.storage.save(<-firstAdmin, to: /storage/currencyAdmin)_36 }_36}
Do not modify smart contract state or emit events in public struct initializers
This is another example of the risks of having publicly accessible parts to your smart contract.
Problem
Data structure definitions in Cadence currently must be declared as public so that they can be used by anyone. Structs do not have the same restrictions that resources have on them, which means that anyone can create a new instance of a struct without going through any authorization.
Solution
Any contract state-modifying operations related to the creation of structs should be contained in resources instead of the initializers of structs.
Example
This used to be a bug in the NBA Top Shot smart contract, so we'll use that as an example. Before, when it created a new play, it would initialize the play record with a struct, which increments the number that tracks the play IDs and emits an event:
_23// Simplified Code_23// BAD_23//_23access(all) contract TopShot {_23_23 // The Record that is used to track every unique play ID_23 access(all) var nextPlayID: UInt32_23_23 access(all) struct Play {_23_23 access(all) let playID: UInt32_23_23 init() {_23_23 self.playID = TopShot.nextPlayID_23_23 // Increment the ID so that it isn't used again_23 TopShot.nextPlayID = TopShot.nextPlayID + 1_23_23 emit PlayCreated(id: self.playID, metadata: metadata)_23 }_23 }_23}
This is a risk because anyone can create the Play struct as many times as they want,
which could increment the nextPlayID field to the max UInt32 value,
effectively preventing new plays from being created. It also would emit bogus events.
This bug was fixed by instead updating the contract state in the admin function that creates the plays.
_34// Update contract state in admin resource functions_34// GOOD_34//_34access(all) contract TopShot {_34_34 // The Record that is used to track every unique play ID_34 access(all) var nextPlayID: UInt32_34_34 access(all) struct Play {_34_34 access(all) let playID: UInt32_34_34 init() {_34 self.playID = TopShot.nextPlayID_34 }_34 }_34_34 access(all) resource Admin {_34_34 // Protected within the private admin resource_34 access(all) fun createPlay() {_34 // Create the new Play_34 var newPlay = Play()_34_34 // Increment the ID so that it isn't used again_34 TopShot.nextPlayID = TopShot.nextPlayID + UInt32(1)_34_34 emit PlayCreated(id: newPlay.playID, metadata: metadata)_34_34 // Store it in the contract storage_34 TopShot.playDatas[newPlay.playID] = newPlay_34 }_34 }_34}