feat: add security best practices #397
Conversation
There was a problem hiding this comment.
Overall, the page provides too little of what this or those (1 and 2) do. Start by adding more concrete examples of pitfalls and possible workarounds or solid recommendations, while also directing users to the aforementioned pages for the full context — adding Callouts with links where appropriate and/or a list of further things to read at the end of the page
| ## Send private data on chain | ||
|
|
||
| Everything is stored on the blockchain. Remember that even if no private data is saved in the transaction, anyone can still get this data. |
There was a problem hiding this comment.
Please, clarify and elaborate. For example, I'd suggest using "privacy sensitive", just "sensitive" or "confidential" over "private", as the latter also implies that something cannot be read and/or is unaccessible.
There was a problem hiding this comment.
I'd rewrite this paragraph, because Remember that even if no private data is saved in the transaction, anyone can still get this data. seems confusing.
You probably meant that the whole smart contract computation is transparent and if you had some confidential values in runtime these could be retrieved with a simple emulation (e.g. https://retracer.ton.org/).
|
|
||
| Everything is stored on the blockchain. Remember that even if no private data is saved in the transaction, anyone can still get this data. | ||
|
|
||
| ## Incorrect use of signed/unsigned integer |
There was a problem hiding this comment.
| ## Incorrect use of signed/unsigned integer | |
| ## Misuse of signed integers |
|
|
||
| Note that [`random()`](/ref/core-random#random) function is pseudo-random and depends on [logical time](https://docs.ton.org/develop/smart-contracts/guidelines/message-delivery-guarantees#what-is-a-logical-time). A hacker can predict random by brute-forcing the logical time in the current block. | ||
|
|
||
| Invalid example: |
There was a problem hiding this comment.
Is there a valid example then?
There was a problem hiding this comment.
there's no 100% reliable and secure way for random generation in TON currently, so if something very important depends on the result (lotteries, gambling, etc), I'd suggest not to build it fully on-chain, or come up with some custom solutions for RNG
| ## Invalid throw values | ||
|
|
||
| Each time the TVM execution stops normally, it stops with exit codes `0` or `1`. | ||
| Although it is done automatically, TVM execution can be interrupted directly in an unexpected way if exit codes `0` and `1` are thrown directly by either `throw(0)` or `throw(1)` command. |
There was a problem hiding this comment.
| ## Invalid throw values | |
| Each time the TVM execution stops normally, it stops with exit codes `0` or `1`. | |
| Although it is done automatically, TVM execution can be interrupted directly in an unexpected way if exit codes `0` and `1` are thrown directly by either `throw(0)` or `throw(1)` command. | |
| ## Throwing exit codes of normal execution | |
| Exit codes $0$ and $1$ indicate normal TVM execution of the compute phase of the transaction. Execution can be unexpectedly aborted by calling [`throw(){:tact}`](https://docs.tact-lang.org/ref/core-debug#throw) (or [similar functions](https://docs.tact-lang.org/ref/core-debug)) directly with exit codes $0$ and $1$, which can make debugging very difficult since this aborted execution would be considered normal. |
| @@ -0,0 +1,144 @@ | |||
| # Security Best Practices | |||
There was a problem hiding this comment.
Some of the examples below demonstrate "harmful actions", while others describe "how-to's". I suggest that there would be another level of headings, and all bad things would be listed under ## Common pitfalls, while all good things would be listed under ## Best practices. All the current h2 (##) headings would become h3 (###) ones.
Alternatively, you might consider combining both under "best practices", since that's the title of the page. In this case, please, name the headings in a positive way (## Use or do this over ## Bad action not to perform) and also provide "valid examples" ("recommendations") whenever possible :)
| ## Send private data on chain | ||
|
|
||
| Everything is stored on the blockchain. Remember that even if no private data is saved in the transaction, anyone can still get this data. |
There was a problem hiding this comment.
I'd rewrite this paragraph, because Remember that even if no private data is saved in the transaction, anyone can still get this data. seems confusing.
You probably meant that the whole smart contract computation is transparent and if you had some confidential values in runtime these could be retrieved with a simple emulation (e.g. https://retracer.ton.org/).
|
|
||
| Note that [`random()`](/ref/core-random#random) function is pseudo-random and depends on [logical time](https://docs.ton.org/develop/smart-contracts/guidelines/message-delivery-guarantees#what-is-a-logical-time). A hacker can predict random by brute-forcing the logical time in the current block. | ||
|
|
||
| Invalid example: |
There was a problem hiding this comment.
there's no 100% reliable and secure way for random generation in TON currently, so if something very important depends on the result (lotteries, gambling, etc), I'd suggest not to build it fully on-chain, or come up with some custom solutions for RNG
|
|
||
| The frontend should handle string parsing from human-friendly formats into machine-readable binary structures. | ||
| This ensures that only optimized, compact messages are sent to the blockchain, reducing computational and storage costs. | ||
| By doing this, you avoid overloading the blockchain with unnecessary processing, thereby improving efficiency. |
There was a problem hiding this comment.
Blockchain will be fine, as well as the smart contract, haha :)
But fees (both gas and forward) will be higher when using some human readable strings instead of binary structs.
| const voteGasUsage = 10000; // precompute with tests | ||
|
|
||
| receive(msg: Vote) { | ||
| require(context().value > getStorageFee(self.voteGasUsage, false), "Not enough gas!"); |
There was a problem hiding this comment.
you probably confused getComputeFee with getStorageFee
|
|
||
| ## Identity validation | ||
|
|
||
| Always validate identity of the sender if needed. This may be achieved through [`Ownable`](/ref/stdlib-ownable) trait or using state init validation for jettons and NFTs. |
There was a problem hiding this comment.
devs rarely have to verify NFTs and Jettons by their stateinits, and it's not even possible usually, because some tokens may have modified code.
The more common pattern is to verify tokens by their addresses (item address for NFTs and wallet address for Jettons)
| ## Replay protection | ||
|
|
||
| Replay protection is a security mechanism to prevent an attacker from reusing a previous message. More about replay protection may be found [here](https://docs.ton.org/develop/smart-contracts/guidelines/external-messages). | ||
|
|
||
| Recommendation: | ||
|
|
||
| ```tact | ||
| message Msg { | ||
| seqno: Int as uint64; | ||
| } | ||
|
|
||
| contract Sample { | ||
| seqno: Int as uint64; | ||
|
|
||
| receive(msg: Msg) { | ||
| require(self.seqno == msg.seqno, "Invalid seqno"); | ||
| self.seqno += 1; | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Replay protection occurs more often in external messages, not in internals, because those are being executed multiple times by default. But even there, the main logic of the smart contract (some values changing in stored data) usually prevents repeated executions already.
I wrote dozens of DeFi smart contracts and didn't ever have to use special seqno in them.
| ## Race condition of messages | ||
|
|
||
| A message cascade can be processed over many blocks. Assume that while one message flow is running, an attacker can initiate a second one in parallel. | ||
| That is, if a property was checked at the beginning (e.g. whether the user has enough tokens), do not assume that at the third stage in the same contract they will still satisfy this property. |
There was a problem hiding this comment.
This one is probably most important and most complex thing across all these bad practices and recommendations.
This does not only include the race condition of two messages sent by different users, but also race conditions of internal messages between contracts in the same system.
I had to think about this a lot while writing and auditing DeFi smart contracts, because any little flaw in this logic can lead to huge loss of funds if a malicious party finds out.
| Recommendation: | ||
|
|
||
| ```tact | ||
| contract JettonWallet { | ||
| balance: Int as coins; | ||
|
|
||
| bounced(src: bounced<TokenTransferInternal>) { | ||
| self.balance += src.amount; | ||
| } | ||
| } | ||
|
|
||
| ``` |
There was a problem hiding this comment.
I'd expand this example to be closer to something real and understandable.
|
This PR was moved to the main repository: tact-lang/tact#1070. Thanks to @pixelplex, @a-bahdanau for coming up with this one, and thanks to @Gusarich for reviewing it here! |
Towards #72