Skip to main content
Version: 1.0

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 fully 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 reference or 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.

If it is a fully authorized account reference, this is problematic, as the fully-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:


_39
...
_39
// BAD CODE
_39
// DO NOT COPY
_39
_39
// Imagine this code is in a contract that uses a `auth(Storage) &Account` parameter
_39
// to authenticate users to transfer NFTs
_39
_39
// They could deploy the contract with an Ethereum-style access control list functionality
_39
_39
access(all)
_39
fun transferNFT(id: UInt64, owner: auth(Storage) &Account) {
_39
assert(owner(id) == owner.address)
_39
_39
transfer(id)
_39
}
_39
_39
// But they could upgrade the function to have the same signature
_39
// so it looks like it is doing the same thing, but they could also drain a little bit
_39
// of FLOW from the user's vault, a totally separate piece of the account that
_39
// should not be accessible in this function
_39
// BAD
_39
_39
access(all)
_39
fun transferNFT(id: UInt64, owner: auth(Storage) &Account) {
_39
assert(owner(id) == owner.address)
_39
_39
transfer(id)
_39
_39
// Sneakily borrow a reference to the user's Flow Token Vault
_39
// and withdraw a bit of FLOW
_39
// BAD
_39
let vaultRef = owner.borrow<&FlowToken.Vault>(/storage/flowTokenVault)!
_39
let stolenTokens <- vaultRef.withdraw(amount: 0.1)
_39
_39
// deposit the stolen funds in the contract owners vault
_39
// BAD
_39
contractVault.deposit(from: <-stolenTokens)
_39
}
_39
...

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 should only be used if it is a very restricted subset of account functionality that is required.

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


_48
// Pseudo-code
_48
_48
// BAD
_48
access(all)
_48
contract Currency {
_48
_48
access(all)
_48
resource Admin {
_48
_48
access(all)
_48
fun mintTokens()
_48
}
_48
_48
// Anyone in the network can call this function
_48
// And use the Admin resource to mint tokens
_48
access(all)
_48
fun createAdmin(): @Admin {
_48
return <-create Admin()
_48
}
_48
}
_48
_48
// This contract makes the admin creation private and in the initializer
_48
// so that only the one who controls the account can mint tokens
_48
// GOOD
_48
access(all)
_48
contract Currency {
_48
_48
access(all)
_48
resource Admin {
_48
_48
access(all)
_48
fun mintTokens()
_48
_48
// Only an admin can create new Admins
_48
access(all)
_48
fun createAdmin(): @Admin {
_48
return <-create Admin()
_48
}
_48
}
_48
_48
init() {
_48
// Create a single admin resource
_48
let firstAdmin <- create Admin()
_48
_48
// Store it in private account storage, so only the admin can use it
_48
self.account.storage.save(<-firstAdmin, to: /storage/currencyAdmin)
_48
}
_48
}

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:


_27
// Simplified Code
_27
// BAD
_27
//
_27
access(all)
_27
contract TopShot {
_27
_27
// The Record that is used to track every unique play ID
_27
access(all)
_27
var nextPlayID: UInt32
_27
_27
access(all)
_27
struct Play {
_27
_27
access(all)
_27
let playID: UInt32
_27
_27
init() {
_27
_27
self.playID = TopShot.nextPlayID
_27
_27
// Increment the ID so that it isn't used again
_27
TopShot.nextPlayID = TopShot.nextPlayID + 1
_27
_27
emit PlayCreated(id: self.playID, metadata: metadata)
_27
}
_27
}
_27
}

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.


_40
// Update contract state in admin resource functions
_40
// GOOD
_40
//
_40
access(all)
_40
contract TopShot {
_40
_40
// The Record that is used to track every unique play ID
_40
access(all)
_40
var nextPlayID: UInt32
_40
_40
access(all)
_40
struct Play {
_40
_40
access(all)
_40
let playID: UInt32
_40
_40
init() {
_40
self.playID = TopShot.nextPlayID
_40
}
_40
}
_40
_40
access(all)
_40
resource Admin {
_40
_40
// Protected within the private admin resource
_40
access(all)
_40
fun createPlay() {
_40
// Create the new Play
_40
var newPlay = Play()
_40
_40
// Increment the ID so that it isn't used again
_40
TopShot.nextPlayID = TopShot.nextPlayID + UInt32(1)
_40
_40
emit PlayCreated(id: newPlay.playID, metadata: metadata)
_40
_40
// Store it in the contract storage
_40
TopShot.playDatas[newPlay.playID] = newPlay
_40
}
_40
}
_40
}