Merge pull request #385 from crytic/ton

TON not so smart contracts

README.md

@@ -27,6 +27,7 @@ Brought to you by [Trail of Bits](https://www.trailofbits.com/), this repository
   - [Cosmos](./not-so-smart-contracts/cosmos)
   - [Substrate](./not-so-smart-contracts/substrate)
   - [Solana](./not-so-smart-contracts/solana)
+  - [TON](./not-so-smart-contracts/ton)
 - [Program Analysis](./program-analysis): Using automated tools to secure contracts
   - [Echidna](./program-analysis/echidna): A fuzzer that checks your contract's properties
   - [Medusa](./program-analysis/medusa/docs/src): A next-gen fuzzer that checks your contract's properties

not-so-smart-contracts/README.md

@@ -7,3 +7,4 @@ This repository contains examples of common smart contract vulnerabilities, incl
 - [Cosmos](./cosmos/README.md)
 - [Solana](./solana/README.md)
 - [Substrate](./substrate/README.md)
+- [TON](./ton/README.md)

not-so-smart-contracts/ton/README.md

@@ -0,0 +1,27 @@
+# (Not So) Smart Contracts
+
+This repository contains examples of common TON smart contract vulnerabilities, featuring code from real smart contracts. Utilize the Not So Smart Contracts to learn about TON vulnerabilities, refer to them during security reviews, and use them as a benchmark for security analysis tools.
+
+## Features
+
+Each _Not So Smart Contract_ consists of a standard set of information:
+
+- Vulnerability type description
+- Attack scenarios to exploit the vulnerability
+- Recommendations to eliminate or mitigate the vulnerability
+- Real-world contracts exhibiting the flaw
+- References to third-party resources providing more information
+
+## Vulnerabilities
+
+| Not So Smart Contract                                          | Description                                                 |
+| -------------------------------------------------------------- | ----------------------------------------------------------- |
+| [Int as boolean](int_as_boolean)                               | Unexpected result of logical operations on the int type     |
+| [Fake Jetton contract](fake_jetton_contract)                   | Any contract can send a `transfer_notification` message     |
+| [Forward TON without gas check](forward_ton_without_gas_check) | Users can drain TON balance of a contract lacking gas check |
+
+## Credits
+
+These examples are developed and maintained by [Trail of Bits](https://www.trailofbits.com/).
+
+If you have any questions, issues, or wish to learn more, join the #ethereum channel on the [Empire Hacking Slack](https://slack.empirehacking.nyc/) or [contact us](https://www.trailofbits.com/contact/) directly.

not-so-smart-contracts/ton/fake_jetton_contract/README.md

@@ -0,0 +1,63 @@
+# Fake Jetton Contract
+
+TON smart contracts use the `transfer_notification` message sent by the receiver's Jetton wallet contract to specify and process a user request along with the transfer of a Jetton. Users add a `forward_payload` to the Jetton `transfer` message when transferring their Jettons, this `forward_payload` is forwarded by the receiver's Jetton wallet contract to the receiver in the `transfer_notification` message. The `transfer_notification` message has the following TL-B schema:
+
+```
+transfer_notification#7362d09c query_id:uint64 amount:(VarUInteger 16)
+                              sender:MsgAddress forward_payload:(Either Cell ^Cell)
+                              = InternalMsgBody;
+```
+
+The `amount` and `sender` are added by the receiver's Jetton wallet contract as the amount of Jettons transferred and the sender of Jettons (owner of the Jetton wallet that sent of the `internal_transfer` message). However, all the other values specified by the user in the `forward_payload` are not parsed or validated by the Jetton wallet contract, they are added as it and sent in the `transfer_notification` message. Therefore, the receiver of the `transfer_notification` message must consider malicious values in the `forward_payload` and validate them properly to prevent any contract state manipulation.
+
+## Example
+
+The following simplified code highlights the lack of token_id validation in the `transfer_notification` message. This contract tracks user deposits by updating the `token0_balances` dictionary entry for the user's address. However, the `transfer_notification` message handler does not verify that the `sender_address` is equal to one of the `token0` or `token1` Jetton wallets owned by this contract. This allows users to manipulate their deposit values by sending the `transfer_notification` message from a fake Jetton wallet contract.
+
+```FunC
+#include "imports/stdlib.fc";
+
+() recv_internal(int my_balance, int msg_value, cell in_msg_full, slice in_msg_body) impure {
+    slice cs = in_msg_full.begin_parse();
+    int flags = cs~load_uint(4);
+    ;; ignore all bounced messages
+    if (is_bounced?(flags)) {
+        return ();
+    }
+    slice sender_address = cs~load_msg_addr(); ;; incorrectly assumed to be Jetton wallet contract owned by this contract
+
+    (cell token0_balances, cell token1_balances) = load_data(); ;; balances dictionaries
+
+    (int op, int query_id) = in_msg_body~load_op_and_query_id();
+
+    if (op == op::transfer_notification) {
+        (int amount, slice from_address) = (in_msg_body~load_coins(), in_msg_body~load_msg_addr());
+        cell forward_payload_ref = in_msg_body~load_ref();
+        slice forward_payload = forward_payload_ref.begin_parse();
+
+        int is_token0? = forward_payload~load_int(1);
+
+        if (is_token0?) {
+            slice balance_before = token0_balances.dict_get?(267, from_address);
+            int balance = balance_before~load_coins();
+            balance = balance + amount;
+            slice balance_after = begin_cell().store_coinds(balance).end_cell().being_parse();
+            token0_balances~dict_set(267, from_address, balance_after);
+        } else {
+            slice balance_before = token1_balances.dict_get?(267, from_address);
+            int balance = balance_before~load_coins();
+            balance = balance + amount;
+            slice balance_after = begin_cell().store_coinds(balance).end_cell().being_parse();
+            token1_balances~dict_set(267, from_address, balance_after);
+        }
+
+        save_data();
+        return ();
+    }
+}
+```
+
+## Mitigations
+
+- Store the address of Jetton wallet contract owned by the current contract at the time of contract initialization and use this stored value to verify the sender of the `transfer_notification` message.
+- Validate all the user provided values in the `forward_payload` instead of trusting users to send correct values.

not-so-smart-contracts/ton/forward_ton_without_gas_check/README.md

@@ -0,0 +1,59 @@
+# Foward TON without gas check
+
+TON smart contracts needs to send TON as gas fee along with the Jetton transfers or any other message they send to another contract. If a contract allows its users to specify the amount of TON to be sent with an outgoing message then it must check that the user supplied enough TON with their message to cover for the transaction fee and the forward TON amount.
+
+If a contract lacks such a gas check then users can specify a higher forward TON amount to drain the TON balance of the smart contract, freezing the smart contract and potentially destrying it.
+
+## Example
+
+The following simplified code highlights the lack of gas check. The following contract implements a `withdraw` operation that allows users to specify a forward TON amount to and forward payload to send with the Jettons. However, this contract does not chekc if used included enough TON with the `withdraw` message to cover for the `withdraw` message transaction, jetton transfer gas fee, and forward TON amount. This allows users to drain the TON balance of the smart contract.
+
+```FunC
+#include "imports/stdlib.fc";
+
+() recv_internal(int my_balance, int msg_value, cell in_msg_full, slice in_msg_body) impure {
+    slice cs = in_msg_full.begin_parse();
+    int flags = cs~load_uint(4);
+    ;; ignore all bounced messages
+    if (is_bounced?(flags)) {
+        return ();
+    }
+    slice sender_address = cs~load_msg_addr(); ;; incorrectly assumed to be Jetton wallet contract owned by this contract
+
+    (int op, int query_id) = in_msg_body~load_op_and_query_id();
+
+    if (op == op::withdraw) {
+        int amount = in_msg_body~load_coins();
+        slice to_address = in_msg_body~load_msg_addr();
+        int forward_ton_amount = in_msg_body~load_coins(); ;; user specified forward TON amount
+        cell forward_payload = begin_cell().store_slice(in_msg_body).end_cell();
+
+        var msg_body = begin_cell()
+            .store_op_and_query_id(op::transfer, query_id)
+            .store_coins(amount)
+            .store_slice(to_address)
+            .store_slice(to_address)
+            .store_uint(0, 1)
+            .store_coins(forward_ton_amount)
+            .store_maybe_ref(forward_payload)
+            .end_cell();
+
+        cell msg = begin_cell()
+            .store_uint(0x18, 6)
+            .store_slice(USDT_WALLET_ADDRESS)
+            .store_coins(JETTON_TRANSFER_GAS + forward_ton_amount) ;; sending user specified forward TON amount
+            .store_uint(1, 1 + 4 + 4 + 64 + 32 + 1 + 1) ;; message parameters
+            .store_ref(ref)
+            .end_cell();
+
+        send_raw_message(msg, 0);
+
+        return ();
+    }
+}
+```
+
+## Mitigations
+
+- Avoid allowing users to specify forward TON amount with the outgoing messages.
+- Always check that the user sent enough TON in the `msg_value` to cover for the current transaction fee and sum of all the outgoing message values.

not-so-smart-contracts/ton/int_as_boolean/README.md

@@ -0,0 +1,90 @@
+# Using int as boolean values
+
+In FunC, booleans are represented as integers; false is represented as 0 and true is represented as -1 (257 ones in binary notation).
+
+Logical operations are done as bitwise operations over the binary representation of the integer values. Notably, The not operation `~` flips all the bits of an integer value; therefore, a non-zero value other than -1 becomes another non-zero value.
+
+When a condition is checked, every non-zero integer is considered a true value. This, combined with the logical operations being bitwise operations, leads to an unexpected behavior of `if` conditions using the logical operations.
+
+## Example
+
+The following simplified code highlights the unexpected behavior of the `~` operator on a non-zero interger value.
+
+```FunC
+#include "imports/stdlib.fc";
+
+() recv_internal(int my_balance, int msg_value, cell in_msg_full, slice in_msg_body) impure {
+    int correct_true = -1;
+    if (correct_true) {
+        ~strdump("correct_true is true"); ;; printed
+    } else {
+        ~strdump("correct_true is false");
+    }
+
+    if (~ correct_true) {
+        ~strdump("~correct_true is true");
+    } else {
+        ~strdump("~correct_true is false"); ;; printed
+    }
+
+    int correct_false = 0;
+    if (correct_false) {
+        ~strdump("correct_false is true");
+    } else {
+        ~strdump("correct_false is false"); ;; printed
+    }
+
+    if (~ correct_false) {
+        ~strdump("~correct_false is true"); ;; printed
+    } else {
+        ~strdump("~correct_false is false");
+    }
+
+    int positive = 10;
+    if (positive) {
+        ~strdump("positive is true"); ;; printed
+    } else {
+        ~strdump("positive is false");
+    }
+
+    if (~ positive) {
+        ~strdump("~positive is true"); ;; printed but unexpected
+    } else {
+        ~strdump("~positive is false");
+    }
+
+    int negative = -10;
+    if (negative) {
+        ~strdump("negative is true"); ;; printed
+    } else {
+        ~strdump("negative is false");
+    }
+
+    if (~ negative) {
+        ~strdump("~negative is true"); ;; printed but unexpected
+    } else {
+        ~strdump("~negative is false");
+    }
+}
+```
+
+The `recv_internal` function above prints the following debug logs:
+
+```
+    #DEBUG#: correct_true is true
+    #DEBUG#: ~correct_true is false
+    #DEBUG#: correct_false is false
+    #DEBUG#: ~correct_false is true
+    #DEBUG#: positive is true
+    #DEBUG#: ~positive is true
+    #DEBUG#: negative is true
+    #DEBUG#: ~negative is true
+```
+
+It demonstrats that the `~ 10` and `~ -10` both evaluate to `true` instead of becoming `false` with the `~` operator.
+
+## Mitigations
+
+- Always use `0` or `-1` in condition checks to get correct results.
+- Be careful with the logical operator usage on non-zero integer values.
+- Implement test cases to verify correct behavior of all condition checks with different interger values.