Skip to main content
Version: 0.42

This is an opinionated list of issues that can be improved if they are found in Cadence code intended for production.

Security and Robustness

Avoid using AuthAccount as a function parameter

Problem

Some may choose to authenticate or perform operations for their users by using the users' account addresses. In order to do this, a commonly seen case would be to pass the user's AuthAccount object as a parameter to a contract function to use for querying the account or storing objects directly. This is problematic, as the AuthAccount object allows access to ALL private areas of the account, for example, all of the user's storage, authorized keys, etc., 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 AuthAccount to authenticate users
_37
// To transfer NFTs
_37
_37
// They could deploy the contract with an Ethereum-style access control list functionality
_37
_37
pub fun transferNFT(id: UInt64, owner: AuthAccount) {
_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
_37
pub fun transferNFT(id: UInt64, owner: AuthAccount) {
_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 AuthAccount object is necessary, such as a cold storage multi-sig, but those cases are extremely rare and AuthAccount usage should still be avoided unless absolutely necessary.

Auth references and capabilities should be avoided

Problem

Authorized references allow downcasting restricted types to their unrestricted type and should be avoided unless necessary. The type that is being restricted could expose functionality that was not intended to be exposed. If the auth keyword is used on local variables they will be references. References are ephemeral and cannot be stored. This prevents any reference casting to be stored under account storage. Additionally, if the auth keyword is used to store a public capability, serious harm could happen since the value could be downcasted to a type that has functionality and values altered.

Example

A commonly seen pattern in NFT smart contracts is including a public borrow function that borrows an auth reference to an NFT (eg. NBA Top Shot). This allows anyone to access the stored metadata or extra fields that weren't part of the NFT standard. While generally safe in most scenarios, not all NFTs are built the same. Some NFTs may have privileged functions that shouldn't be exposed by this method, so please be cautious and mindful when imitating NFT projects that use this pattern.

Another Example

When we create a public capability for our FungibleToken.Vault we do not use an auth capability:


_10
// GOOD: Create a public capability to the Vault that only exposes
_10
// the balance field through the Balance interface
_10
signer.link<&FlowToken.Vault{FungibleToken.Balance}>(
_10
/public/flowTokenBalance,
_10
target: /storage/flowTokenVault
_10
)

If we were to use an authorized type for the capability, like so:


_10
// BAD: Create an Authorized public capability to the Vault that only exposes
_10
// the balance field through the Balance interface
_10
// Authorized referenced can be downcasted to their unrestricted types, which is dangerous
_10
signer.link<auth &FlowToken.Vault{FungibleToken.Balance}>(
_10
/public/flowTokenBalance,
_10
target: /storage/flowTokenVault
_10
)

Then anyone in the network could take that restricted reference that is only supposed to expose the balance field and downcast it to expose the withdraw field and steal all our money!


_10
// Exploit of the auth capability to expose withdraw
_10
let balanceRef = getAccount(account)
_10
.getCapability<auth &FlowToken.Vault{FungibleToken.Balance}>(/public/flowTokenBalance)
_10
.borrow()!
_10
_10
let fullVaultRef = balanceRef as! &FlowToken.Vault
_10
_10
// Withdraw the newly exposed funds
_10
let stolenFunds <- fullVaultRef.withdraw(amount: 1000000)

Events from resources may not be unique

Problem

Public functions in a contract can be called by anyone, e.g. any other contract or any transaction. If that function creates a resource, and that resource has functions that emit events, that means any account can create an instance of that resource and emit those events. If those events are meant to indicate actions taken using a single instance of that resource (eg. admin object, registry), or instances created through a particular workflow, it's possible that events from other instances may be mixed in with the ones you're querying for - making the event log search and management more cumbersome.

Solution

To fix this, if there should be only a single instance of the resource, it should be created and link()ed to a public path in an admin account's storage during the contracts's initializer.

Access Control

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 they are all 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.

Array or dictionary fields should be private

info

This anti-pattern has been addressed with FLIP #703

Problem

This is a specific case of "Public Functions And Fields Should Be Avoided", above. Public array or dictionary fields are not directly over-writable, but their members can be accessed and overwritten if the field is public. This could potentially result in security vulnerabilities for the contract if these fields are mistakenly made public.

Ex:


_10
pub contract Array {
_10
// array is intended to be initialized to something constant
_10
pub let shouldBeConstantArray: [Int]
_10
}

Anyone could use a transaction like this to modify it:


_10
import Array from 0x01
_10
_10
transaction {
_10
execute {
_10
Array.shouldbeConstantArray[0] = 1000
_10
}
_10
}

Solution

Make sure that any array or dictionary fields in contracts, structs, or resources are access(contract) or access(self) unless they need to be intentionally made public.


_10
pub contract Array {
_10
// array is inteded to be initialized to something constant
_10
access(self) let shouldBeConstantArray: [Int]
_10
}

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 init() method, and then a new creation function can be potentially included within the admin resource, if necessary. The admin resource can then be link()ed to a private path in an admin's account storage during the contract's initializer.

Example


_36
// Pseudo-code
_36
_36
// BAD
_36
pub contract Currency {
_36
pub resource Admin {
_36
pub fun mintTokens()
_36
}
_36
_36
// Anyone in the network can call this function
_36
// And use the Admin resource to mint tokens
_36
pub 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
_36
pub contract Currency {
_36
pub resource Admin {
_36
pub fun mintTokens()
_36
_36
// Only an admin can create new Admins
_36
pub 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 in `init` so only the admin can use it
_36
self.account.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 restricted 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
//
_23
pub contract TopShot {
_23
_23
// The Record that is used to track every unique play ID
_23
pub var nextPlayID: UInt32
_23
_23
pub struct Play {
_23
_23
pub 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
//
_34
pub contract TopShot {
_34
_34
// The Record that is used to track every unique play ID
_34
pub var nextPlayID: UInt32
_34
_34
pub struct Play {
_34
_34
pub let playID: UInt32
_34
_34
init() {
_34
self.playID = TopShot.nextPlayID
_34
}
_34
}
_34
_34
pub resource Admin {
_34
_34
// Protected within the private admin resource
_34
pub 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
}