This checklist is co-authored by Ken Huang, @bluesign aka Deniz, and Julien@flowstarter
This checklist is used as an internal review and does not replace third-party reviews. The project team is strongly encouraged to engage a third party to review the code before the mainnet deployment of the code. The Flow team is currently reviewing the code as well so it is a plus.
1： Access Modifier Review
Cadence variables, constants, and functions all have the access control modifier to define the read and write scope.
If the modifier is not used correctly, there will be security risks. The auditor can check the access modifiers to see if they are used correctly.
`let` can have write scope to elements, if it is `array` or `dictionary`. This one is unfortunately very common mistake
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. Make sure that any array or dictionary fields in contracts, structs, or resources are access(contract), access(account) or access(self) unless they need to be intentionally made public.
Example:
pub contract Array {
// array is intended to be initialized to something constant
pub let shouldBeConstantArray: [Int]
}
Anyone could use a transaction like this to modify it:
import Array from 0x01
transaction {
execute {
Array.shouldbeConstantArray[0] = 1000
}
}
Public Admin Resource Creation Functions Are Unsafe
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 function should not be public.
To fix this, a single instance of that resource created using it then link()ed to a private path in an admin account’s storage during the contract’s initializer. If the code to create the resource is complex enough that it needs its own function, the function that creates the resource should use access(contract) as its access modifier.
Public Capability Fields Are A Security Hole
The values of public fields can be copied. Capabilities are value types. Anyone who receives a Capability can use it. So if they can copy it from a public field they can call the functions that it exposes.
This almost certainly is not what you want if a capability has been stored in this way. For public access to a capability, place it in an accounts public area.
Users Might Rebind Restricted Public Capability Paths
A public capability on a user account that is a variant of a standard resource (such as a Vault) that has additional logic implemented to restrict or alter its behavior can be replaced by that user at the same path with another resource that does not have those alterations.
To fix this, make sure to carefully specify the type of the capability that you are expecting in any code that accesses it.
Declarations of structures, resources, events, and contracts can only be public. However, even though the declarations/types are publicly visible, resources can only be created from inside the contract they are declared in.
2：Capability Access Control Review
Capabilities are identified by a path and link to a target path, not directly to an object. Capabilities are either public (any user can get access), or private (access to/from the authorized user is necessary). Public capabilities are created using public paths, i.e. they have the domain public. After creation, they can be obtained from both authorized accounts (AuthAccount) and public accounts (PublicAccount).
Private capabilities are created using private paths, i.e. they have the domain private. After creation they can be obtained from authorized accounts (AuthAccount), but not from public accounts (PublicAccount).
Once a capability is created and obtained, it can be borrowed to get a reference to the stored object. When a capability is created, a type is specified that determines what type the capability can be borrowed. This allows exposing and hiding certain functionality of a stored object.
Capabilities can be removed using the unlink function of an authorized account (AuthAccount).
The auditor can check to see if the capability should be defined as private instead of public and if the capability should be removed using the unlink function when no longer needed.
Capabilities (even if they are private, should not be stored publicly accessible) They can be copied (as they are structs)
Make sure to assign each capability a unique private path so you can revoke (unlink) later.
Using `capability receiver` is also a good pattern.
Contract developers must be aware that a capability provided by a user’s account can potentially be unlinked causing code that borrows the resource to fail. Fallbacks should be provided if the functionality is essential. Also, a capability could be replaced with a different Type which would pass a check() but fail to be cast on borrow() Again fallbacks should be provided if the code’s functionality is essential.
3: Four phases of a transaction: Preparation, preconditions, execution, and postconditions Review
In Flow, there are 3 types of code that can interact with Flow blockchain: Contract, Transaction, and Scripts. The contract is deployed to the Flow blockchain and can change blockchain state if executed or invoked. The transaction is not deployed to the Flow and can interact with the Contract deployed on the Flow blockchain to change the state of the chain. The script can read the state of the blockchain but will not change or write the state to the Blockchain.
So, for your project, if you do have the Transaction code that will be invoked by your frontend code (such as ReactApp), it is better to check the Transaction code to see if there are any potential security issues and check the Postconditions to see if the Transaction meet your defined business logic. Of course, keep in mind that your Contact needs to be tested for all positive or negative transactions to preempt hacker’s transactions which could exploit the security holes in your contract. One potential pattern is to have your Contract to whitelisting allowed transactions with the hash of transaction code at wallet level or at the contact level.
Blocto wallet is working with this as more of a warning. Currently all transactions show a lot of code that is no doubt kind of scary for non-technical users. Their solution is more about hiding that and instead showing this is a transaction created by the team and has been run xxxx times by other users’ or something similar if a transaction is in their whitelist
https://github.com/portto/flow-transactions/tree/main/transactions/BloctoSwap
A transaction has 4 optional phases illustrated using the following code.
The auditor can check if the code block is in the right phases.
`Post conditions` are rarely used but it is one of the most important parts of the transactions. (for user) and also for `contract functions`.
In the Postcondition, you need to check for invariant, and also for the balance (total balance or self.balance after execution), and other Postconditions. For example, you should check Post condition with what the execution result should be. if you are buying let’s say 1 NFT with id X for 10 FLOW, the balance should be 10 FLOW less, and you should have in your collection the purchased NFT with that id X.
transaction {
prepare(signer1: AuthAccount) {
// Access signing accounts for this transaction.
//
// Avoid logic that does not need access to signing accounts.
//
// Signing accounts can’t be accessed anywhere else in the transaction.
}
pre {
// Define conditions that must be true
// for this transaction to execute.
}
execute {
// The main transaction logic goes here, but you can access
// any public information or resources published by any account.
}
post {
// Define the expected state of things
// as they should be after the transaction is executed.
//
// Also used to provide information about what changes
// this transaction will make to accounts in this transaction.
}
}
4：Roles Review
A cadence transaction can be built using 3 different roles: proposer, authorizer, and payer as illustrated below:
> flow transactions build ./transaction.cdc “Meow” \
— authorizer alice \
— proposer bob \
— payer charlie \
— filter payload — save built.rlp:
The auditor can work with the project team to define “Separation of Duty” and “Least privileges” security principles to define the roles for authorizer, proposer, and payer.
One thing to be careful here is, when signing the envelope `payer` is also an `authorizer` as far as signing goes, so if you are signing for someone else as a `payer` be careful to check, they don’t set you as `authorizer` to gain access to your account. (also same goes for authorizer (as proposer), always a rule of thumb is to check what you are signing)
5: Events From Resources Might Not Be Unique
A public function on a contract that creates a resource can be called by any account.
If that resource has functions that emit events, 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 (for example an admin object, switchboard, or registry), or instances created through a particular workflow, then this will make event log search and management harder because events from other instances may be mixed in with the ones that you wish to examine.
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.
6:Named Value Fields Are Preferable
Where contracts, resources, and scripts all have to repeatedly refer to the same constant values, entering these manually is a potential source of error. Rather than typing the values manually in functions, transactions and scripts, store the values once in fields of the contract responsible and then access them via those fields. This is the Named Value Field pattern
Add an access(all) field, e.g. a Path , to the contract responsible for the value, and set it in the contract’s initializer. Then refer to that value via this public field rather than specifying it manually.
7：Business Logic security review
Depending on the business logic, the auditor can check security aspects of the following:
If your contract code can tolerate the about 10 second variation, use block timestamps. If not, you may need to consider more exotic solutions (time oracles, etc.).
Whichever method you use, be careful not to hardcode any assumptions about block rates production rates into your code, on-chain or off, in a way that cannot be updated later.
On-chain auctions and similar mechanisms should always have an extension mechanism. If someone bids at the last moment (which is easier to do with a block production attack), the end time for the auction extends (if necessary) to N minutes past the last bid. (10 minutes, 30 minutes, an hour). As N increases, this becomes more secure: N=5 should be more than enough. with the current parameters of the Flow blockchain.
- Randomness
There is no reliable random in cadence ( unsafe random is also neither safe nor uniform, any business logic relying on random should be off-chain )
In the future, it is desirable to leverage Chainlink VRF for randomness
https://docs.chain.link/docs/chainlink-vrf/
- Withdraw and Deposit logic
- Pricing information
8: Testing requirements from Flow Team
The Flow team has high level guidance for contract testing, briefly, the following is needed:
- Every publicly exposed feature of a contract and its resources should have unit tests that check both for success with the correct input and for failure with incorrect input. These tests should be capable of being run locally with the Flow emulator, with no or minimal extra resources or configuration, and with a single command.
- Each user story or workflow that uses the smart contracts should have an integration test that ensures that the series of steps required to complete it does so successfully with test data.
For manual testing prior to approval for deployment to mainnet the code must be deployed on the testnet and used for two weeks (use load testing to generate load is needed).
This is to test the code in conditions as close to those it will be used in on mainnet if and when it is deployed there.
There are currently JavaScript and Go testing frameworks you can use to create tests for your smart contract code:
https://github.com/onflow/flow-js-testing
https://github.com/onflow/flow-core-contracts/tree/master/lib/go/test
https://github.com/onflow/flow-nft/tree/master/lib/go/test
https://github.com/onflow/flow-ft/tree/master/lib/go/test
9: Stress testing and DoS Testing
The availability of the system is one aspect of security. Stress testing can be conducted to see availability and response time. The flow team has the guidance
After your app has been approved and deployed, a period of observation will begin. During this time the Flow team will record how your application handles traffic, to ensure there aren’t any problems that could affect the wider network or your application’s user experience.
During the observation period, the Flow team likes to see your application handle non-trivial amounts of traffic. If the Flow team does not see enough traffic, they may delay Mainnet approval.
Including your own automated load testing or load-testing metrics could help your application get approval for Mainnet faster. The Flow team has some knowledge of problematic patterns that could cause problems, so please reach out to them before implementing your idea if you’re unsure!
If you perform a bigger test, please send the Flow team a message on Discord (you can tag @Flow Team in the #developers channel). You may also want to use the emulator to perform stress testing. You can configure the emulator with an artificial block delay to simulate real network latency.
Keep in mind that DoS attacks are serious security issues, DoS risk should be reviewed with business logic.
Also, storage has a cost in Flow, don’t let people store stuff on your account, always try to push storage to the user account. Handle indexing off-chain. For example: if you are making an auction contract, don’t store all bids, just store the winning bid, for rest inform network with events.
