Skip to content

Commit

Permalink
Merge branch 'main' into vishal/hcu
Browse files Browse the repository at this point in the history
  • Loading branch information
@vishalchangrani
vishalchangrani authored Jul 31, 2023
2 parents 6cc7581 + 6b87552 commit 8830c96
Show file tree
Hide file tree
Showing 48 changed files with 1,393 additions and 894 deletions.
112 changes: 24 additions & 88 deletions docs/cadence/anti-patterns.mdx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ which provides the opportunity for bad actors to take advantage of.
// They could deploy the contract with an Ethereum-style access control list functionality
pub fun transferNFT(id: UInt64, owner: AuthAccount) {
access(all) fun transferNFT(id: UInt64, owner: AuthAccount) {
assert(owner(id) == owner.address)
transfer(id)
Expand All @@ -43,7 +43,7 @@ pub fun transferNFT(id: UInt64, owner: AuthAccount) {
// should not be accessible in this function
// BAD
pub fun transferNFT(id: UInt64, owner: AuthAccount) {
access(all) fun transferNFT(id: UInt64, owner: AuthAccount) {
assert(owner(id) == owner.address)
transfer(id)
Expand All @@ -70,70 +70,6 @@ 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](./language/references.mdx) 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](https://github.com/dapperlabs/nba-smart-contracts/blob/95fe72b7e94f43c9eff28412ce3642b69dcd8cd5/contracts/TopShot.cdc#L889-L906)).
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:

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

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

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

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!

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

## Events from resources may not be unique

### Problem
Expand Down Expand Up @@ -201,9 +137,9 @@ if these fields are mistakenly made public.
Ex:

```cadence
pub contract Array {
access(all) contract Array {
// array is intended to be initialized to something constant
pub let shouldBeConstantArray: [Int]
access(all) let shouldBeConstantArray: [Int]
}
```

Expand All @@ -225,7 +161,7 @@ Make sure that any array or dictionary fields in contracts, structs, or resource
are `access(contract)` or `access(self)` unless they need to be intentionally made public.

```cadence
pub contract Array {
access(all) contract Array {
// array is inteded to be initialized to something constant
access(self) let shouldBeConstantArray: [Int]
}
Expand All @@ -252,27 +188,27 @@ The admin resource can then be `link()`ed to a private path in an admin's accoun
// Pseudo-code
// BAD
pub contract Currency {
pub resource Admin {
pub fun mintTokens()
access(all) contract Currency {
access(all) resource Admin {
access(all) fun mintTokens()
}
// Anyone in the network can call this function
// And use the Admin resource to mint tokens
pub fun createAdmin(): @Admin {
access(all) fun createAdmin(): @Admin {
return <-create Admin()
}
}
// This contract makes the admin creation private and in the initializer
// so that only the one who controls the account can mint tokens
// GOOD
pub contract Currency {
pub resource Admin {
pub fun mintTokens()
access(all) contract Currency {
access(all) resource Admin {
access(all) fun mintTokens()
// Only an admin can create new Admins
pub fun createAdmin(): @Admin {
access(all) fun createAdmin(): @Admin {
return <-create Admin()
}
}
Expand Down Expand Up @@ -300,7 +236,7 @@ which means that anyone can create a new instance of a struct without going thro
### Solution

Any contract state-modifying operations related to the creation of structs
should be contained in restricted resources instead of the initializers of structs.
should be contained in resources instead of the initializers of structs.

### Example

Expand All @@ -313,14 +249,14 @@ which increments the number that tracks the play IDs and emits an event:
// Simplified Code
// BAD
//
pub contract TopShot {
access(all) contract TopShot {
// The Record that is used to track every unique play ID
pub var nextPlayID: UInt32
access(all) var nextPlayID: UInt32
pub struct Play {
access(all) struct Play {
pub let playID: UInt32
access(all) let playID: UInt32
init() {
Expand Down Expand Up @@ -348,24 +284,24 @@ that creates the plays.
// Update contract state in admin resource functions
// GOOD
//
pub contract TopShot {
access(all) contract TopShot {
// The Record that is used to track every unique play ID
pub var nextPlayID: UInt32
access(all) var nextPlayID: UInt32
pub struct Play {
access(all) struct Play {
pub let playID: UInt32
access(all) let playID: UInt32
init() {
self.playID = TopShot.nextPlayID
}
}
pub resource Admin {
access(all) resource Admin {
// Protected within the private admin resource
pub fun createPlay() {
access(all) fun createPlay() {
// Create the new Play
var newPlay = Play()
Expand Down
64 changes: 30 additions & 34 deletions docs/cadence/design-patterns.mdx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ See [Wikipedia's page on magic numbers](https://en.wikipedia.org/wiki/Magic_numb

### Solution

Add a public (`pub`), constant (`let`) field, e.g. a `Path` , to the contract responsible for the value,
Add a public (`access(all)`), constant (`let`) field, e.g. a `Path` , to the contract responsible for the value,
and set it in the contract's initializer.
Refer to that value via this public field rather than specifying it manually.

Expand All @@ -38,8 +38,8 @@ Example Snippet:
```cadence
// BAD Practice: Do not hard code storage paths
pub contract NamedFields {
pub resource Test {}
access(all) contract NamedFields {
access(all) resource Test {}
init() {
// BAD: Hard-coded storage path
Expand All @@ -49,11 +49,11 @@ pub contract NamedFields {
// GOOD practice: Instead, use a field
//
pub contract NamedFields {
pub resource Test {}
access(all) contract NamedFields {
access(all) resource Test {}
// GOOD: field storage path
pub let TestStoragePath: StoragePath
access(all) let TestStoragePath: StoragePath
init() {
// assign and access the field here and in transactions
Expand Down Expand Up @@ -81,7 +81,7 @@ Your contract, resource or struct has a field or resource that will need to be r
Make sure that the field can be accessed from a script (using a `PublicAccount`)
rather than requiring a transaction (using an `AuthAccount`).
This saves the time and fees required to read a property using a transaction.
Making the field or function `pub` and exposing it via a `/public/` capability will allow this.
Making the field or function `access(all)` and exposing it via a `/public/` capability will allow this.

Be careful not to expose any data or functionality that should be kept private when doing so.

Expand All @@ -92,7 +92,7 @@ Example:
access(self) let totalSupply: UFix64
// GOOD: Field is public, so it can be read and used by anyone
pub let totalSupply: UFix64
access(all) let totalSupply: UFix64
```

## Script-Accessible report
Expand All @@ -118,25 +118,25 @@ See [Script-Accessible public field/function](#script-accessible-public-fieldfun
### Example Code

```cadence
pub contract AContract {
pub let BResourceStoragePath: StoragePath
pub let BResourcePublicPath: PublicPath
access(all) contract AContract {
access(all) let BResourceStoragePath: StoragePath
access(all) let BResourcePublicPath: PublicPath
init() {
self.BResourceStoragePath = /storage/BResource
self.BResourcePublicPath = /public/BResource
}
// Resource definition
pub resource BResource {
pub var c: UInt64
pub var d: String
access(all) resource BResource {
access(all) var c: UInt64
access(all) var d: String
// Generate a struct with the same fields
// to return when a script wants to see the fields of the resource
// without having to return the actual resource
pub fun generateReport(): BReportStruct {
access(all) fun generateReport(): BReportStruct {
return BReportStruct(c: self.c, d: self.d)
}
Expand All @@ -147,9 +147,9 @@ pub contract AContract {
}
// Define a struct with the same fields as the resource
pub struct BReportStruct {
pub var c: UInt64
pub var d: String
access(all) struct BReportStruct {
access(all) var c: UInt64
access(all) var d: String
init(c: UInt64, d: String) {
self.c = c
Expand All @@ -172,7 +172,7 @@ transaction {
import AContract from 0xAContract
// Return the struct with a script
pub fun main(account: Address): AContract.BReportStruct {
access(all) fun main(account: Address): AContract.BReportStruct {
// borrow the resource
let b = getAccount(account)
.getCapability(AContract.BResourcePublicPath)
Expand Down Expand Up @@ -225,12 +225,12 @@ All fields, functions, types, variables, etc., need to have names that clearly d
```cadence
// BAD: Unclear naming
//
pub contract Tax {
access(all) contract Tax {
// Do not use abbreviations unless absolutely necessary
pub var pcnt: UFix64
access(all) var pcnt: UFix64
// Not clear what the function is calculating or what the parameter should be
pub fun calculate(num: UFix64): UFix64 {
access(all) fun calculate(num: UFix64): UFix64 {
// What total is this referring to?
let total = num + (num * self.pcnt)
Expand All @@ -240,13 +240,13 @@ pub contract Tax {
// GOOD: Clear naming
//
pub contract TaxUtilities {
access(all) contract TaxUtilities {
// Clearly states what the field is for
pub var taxPercentage: UFix64
access(all) var taxPercentage: UFix64
// Clearly states that this function calculates the
// total cost after tax
pub fun calculateTotalCostPlusTax(preTaxCost: UFix64): UFix64 {
access(all) fun calculateTotalCostPlusTax(preTaxCost: UFix64): UFix64 {
let postTaxCost = preTaxCost + (preTaxCost * self.taxPercentage)
return postTaxCost
Expand All @@ -267,15 +267,11 @@ Therefore, it is important to be explicit when getting objects or references to

### Solution

A good example of when the code should specify the type being restricted is checking the FLOW balance:
The code must borrow `&FlowToken.Vault{FungibleToken.Balance}`, in order to ensure that it gets a FLOW token balance,
A good example of when the code should specify the type is checking the FLOW balance:
The code must borrow `&FlowToken.Vault`, in order to ensure that it gets a FLOW token balance,
and not just `&{FungibleToken.Balance}`, any balance – the user could store another object
that conforms to the balance interface and return whatever value as the amount.

When the developer does not care what the concrete type is, they should explicitly indicate that
by using `&AnyResource{Receiver}` instead of `&{Receiver}`.
In the latter case, `AnyResource` is implicit, but not as clear as the former case.

## Plural names for arrays and maps are preferable

e.g. `accounts` rather than `account` for an array of accounts.
Expand Down Expand Up @@ -303,7 +299,7 @@ This could be used when purchasing an NFT to verify that the NFT was deposited i
transaction {
pub let buyerCollectionRef: &NonFungibleToken.Collection
access(all) let buyerCollectionRef: &NonFungibleToken.Collection
prepare(acct: AuthAccount) {
Expand Down Expand Up @@ -426,7 +422,7 @@ and emits an `InboxValueUnpublished` event that the recipient can listen for off
It is also important to note that the recipient becomes the owner of the capability object once they have claimed it,
and can thus store it or copy it anywhere they have access to.
This means providers should only publish capabilities to recipients they trust to use them properly,
or limit the type with which the capability is restricted in order to only give recipients access to the functionality
or limit the type with which the capability is authorized in order to only give recipients access to the functionality
that the provider is willing to allow them to copy.

## Capability Revocation
Expand Down Expand Up @@ -483,7 +479,7 @@ transaction {
signer.unlink(/public/exampleTokenReceiver)
}
signer.link<&ExampleToken.Vault{FungibleToken.Receiver}>(
signer.link<&ExampleToken.Vault>(
/public/exampleTokenReceiver,
target: /storage/exampleTokenVault
)
Expand Down
Loading

0 comments on commit 8830c96

Please sign in to comment.