- Go back to TOP(インデックスページへ)
- Avoid using fully authorized account references as a function parameter
- Public functions and fields should be avoided
- Capability-Typed public fields are a security hole
- Public admin resource creation functions are unsafe
- Do not modify smart contract state or emit events in public struct initializers
これは、本番環境向けのCadenceコードで発見された場合、改善できる問題の意見の多いリストです。
Avoid using fully authorized account references as a function parameter
Problem
開発者は、ユーザーのアカウント参照またはアドレスを使用して、ユーザーの認証や操作を行うことができます。これを行うには、認証済みアカウント参照型を持つ関数にパラメータを追加します（auth(...) &Account）。認証済みアカウント参照は、トランザクションに署名することでのみ取得できるためです。これが完全に認証されたアカウント参照である場合、これは問題となります。なぜなら、完全に認証されたアカウント参照は、アカウント上のいくつかの機密性の高い操作、例えばストレージへの書き込みなどへのアクセスを許可するからです。これは、悪意のあるユーザーに悪用される可能性があることを意味します。
Example:
...
// BAD CODE
// DO NOT COPY
// Imagine this code is in a contract that uses a `auth(Storage) &Account` parameter
// to authenticate users to transfer NFTs
// They could deploy the contract with an Ethereum-style access control list functionality
access(all)
fun transferNFT(id: UInt64, owner: auth(Storage) &Account) {
assert(owner(id) == owner.address)
transfer(id)
}
// But they could upgrade the function to have the same signature
// so it looks like it is doing the same thing, but they could also drain a little bit
// of FLOW from the user's vault, a totally separate piece of the account that
// should not be accessible in this function
// BAD
access(all)
fun transferNFT(id: UInt64, owner: auth(Storage) &Account) {
assert(owner(id) == owner.address)
transfer(id)
// Sneakily borrow a reference to the user's Flow Token Vault
// and withdraw a bit of FLOW
// BAD
let vaultRef = owner.borrow<&FlowToken.Vault>(/storage/flowTokenVault)!
let stolenTokens <- vaultRef.withdraw(amount: 0.1)
// deposit the stolen funds in the contract owners vault
// BAD
contractVault.deposit(from: <-stolenTokens)
}
...
Solution
プロジェクトは、リソースやCapabilityを認証オブジェクトとして使用するなど、ユーザーを認証する他の方法を見つけるべきです。また、ストレージやリンク操作のほとんどは、コントラクトユーティリティ関数内ではなく、トランザクション本体内で実行することを想定すべきです。コールドストレージのマルチシグなど、認証済みアカウント参照（auth(...) &Account）を使用する必要があるシナリオもありますが、そのようなケースはまれであり、必要なアカウントの機能(account functionality)が非常に制限されたサブセットである場合にのみ使用すべきです。
Public functions and fields should be avoided
Problem
コードを構築する際には、アクセス修飾子を常に把握し、公開すべきもののみを公開するようにしてください。誤って公開されたフィールドは、セキュリティ上の脆弱性となり得ます。Solution
スマートコントラクトを記述する際には、すべてのフィールドと関数を確認し、権限（access(E)）によるアクセスが必要であることを確認するか、または、必要でない限り、access(self)、access(contract)、またはaccess(account)などの非公開アクセス修飾子を使用してください。Capability-Typed public fields are a security hole
これは、上記の「パブリック関数およびフィールドは避けるべき」の特別なケースです。Problem
パブリックフィールドの値はコピーすることができます。 Capabilityは値型なので、パブリックフィールドとして使用されている場合、誰でもフィールドからコピーして、そのCapabilityが公開している関数を呼び出すことができます。 コントラクトまたはリソースのフィールドとしてこの方法でCapabilityが保存されている場合、これはおそらく望む結果ではないでしょう。Solution
Capabilityへのパブリックアクセスを可能にするには、この期待結果が明確になるよう、アカウントのパブリックエリアに配置します。Public admin resource creation functions are unsafe
これは、上記の「パブリック関数およびフィールドは避けるべき」の特別なケースです。
Problem
リソースを作成するスマートコントラクト上のパブリック関数は、どのアカウントでも呼び出すことができます。そのリソースが管理機能へのアクセスを提供している場合、作成関数はパブリックにすべきではありません。Solution
これを修正するには、スマートコントラクトの初期化時にそのリソースの単一インスタンスを作成し、必要に応じて、新しい作成関数を管理リソース内に含めることができます。Example
// 疑似コード
// BAD
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
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
access(all)
contract Currency {
access(all)
resource Admin {
access(all)
fun mintTokens()
// Only an admin can create new Admins
access(all)
fun createAdmin(): @Admin {
return <-create Admin()
}
}
init() {
// Create a single admin resource
let firstAdmin <- create Admin()
// Store it in private account storage, so only the admin can use it
self.account.storage.save(<-firstAdmin, to: /storage/currencyAdmin)
}
}
Do not modify smart contract state or emit events in public struct initializers
これは、スマートコントラクトに一般公開されたアクセス可能な部分があることのリスクを示すもう一つの例です。
Problem
現在、Cadenceにおけるデータ構造の定義(原文: Data structure definitions)は、誰でも使用できるように「パブリック」として宣言しなければなりません。構造体にはリソースのような制限がないため、認証(authorization)手続きを経ることなく誰でも新しい構造体のインスタンスを作成することができます。Solution
構造体の作成に関連する、状態を変更する操作はすべて、構造体のイニシャライザー(init関数)ではなくリソースに格納すべきである。Example
これはかつてはNBA Top Shotスマートコントラクトのバグでした。これを例として説明します。以前は、新しい(NBAの)プレイが作成されると、(NBAの)プレイのレコードが構造体で初期化され、プレイIDを追跡する数値がインクリメントされ、イベントが発行されていました。// Simplified Code
// BAD
//
access(all)
contract TopShot {
// The Record that is used to track every unique play ID
access(all)
var nextPlayID: UInt32
access(all)
struct Play {
access(all)
let playID: UInt32
init() {
self.playID = TopShot.nextPlayID
// Increment the ID so that it isn't used again
TopShot.nextPlayID = TopShot.nextPlayID + 1
emit PlayCreated(id: self.playID, metadata: metadata)
}
}
}
これはリスクです。なぜなら、誰でも好きなだけPlay構造体を生成することができ、nextPlayIDフィールドが最大UInt32値まで増加し、事実上、新しいプレイが作成できなくなる可能性があるからです。また、偽のイベントが発行されることにもなります。
このバグは、代わりにプレイを作成する管理機能(admin function)でコントラクトのstateを更新することで修正されました。
// Update contract state in admin resource functions
// GOOD
//
access(all)
contract TopShot {
// The Record that is used to track every unique play ID
access(all)
var nextPlayID: UInt32
access(all)
struct Play {
access(all)
let playID: UInt32
init() {
self.playID = TopShot.nextPlayID
}
}
access(all)
resource Admin {
// Protected within the private admin resource
access(all)
fun createPlay() {
// Create the new Play
var newPlay = Play()
// Increment the ID so that it isn't used again
TopShot.nextPlayID = TopShot.nextPlayID + UInt32(1)
emit PlayCreated(id: newPlay.playID, metadata: metadata)
// Store it in the contract storage
TopShot.playDatas[newPlay.playID] = newPlay
}
}
}
翻訳元->https://cadence-lang.org/docs/anti-patterns