TON-EVM Bridge

CertiK19 Jan 2023Files named in the report

Audit

CertiK reviewed TON (TON-EVM Bridge) on 19 Jan 2023: 23 findings, 0 critical; 8 marked fixed, 15 open.

No open critical findings

Open the report Open TON’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings23

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	1	0
Medium	2	2
Low	12	6

HighAll the money locked in bridge can be extracted vOpen

TON contract `bridge_code` can `execute_voting()` received from the Bridge Governance (`multisig-code` owned by a set of oracles). According to the [ETH-TON bridge description](https://github.com/ton-blockchain/TIPs/issues/24#issuecomment-899768466), each oracle puts a significant amount of TON coins (stake) on the governing smart contract (`multisig-code`). The initial stake is 100k TON coins per oracle. Solidity contract `Bridge` can mint TONCOINs via `voteForMinting()`. In case of oracles collusion: 1. They can mint unlimited amount of ERC20 TONCOINs and sell them on DEX/CEX. The profit depends on the available liquidity and order book. 2. They can extract all the locked TONs from `bridge_code` via common "swap" operation. Reasonable if locked amount is bigger than the total stake. At the moment of writing total stake is 900k TONs, `total_locked` is 11M TONs. At the same time 1. The Bridge Governance can't be reassigned to another contract by Validators because that will automatically "unlock" the stakes. 2. Oracles set on ETH side can't be updated because it requires the signatures of old Bridge Governance participants. 3. If stakes unlocked, the oracles majority can steal minority's stakes. So, "out-of-control" Bridge can't be shut down. New Bridge has to be deployed and will work in parallel. The Bridge assumes that the majority of oracles follow the rules, despite the fact they are incentivized to do not. **Each oracle has to be trusted**, otherwise the malicious actor can stake from many accounts to get the majority.

MediumLack of `block.chainid` in signed data can lead tOpen

The message signed by oracles in one chain (Ethereum mainnet, BSC mainnet, Ethereum testnet, etc.) can be replayed by the attacker in another one, if the Bridge address is the same and oracle keys are the same. The replay attack can also be executed in case of chain fork (like Ethereum/Ethereum Classic).

MediumOracles threshold is rounded downOpen

```solidity=19 require(signatures.length >= 2 * oraclesSet.length / 3, "Not enough signatures"); ``` The check is expected to pass if at least 2/3 or signatures were obtained. However, due to rounding down it can pass with less. For example, with 4 oracles only 2 signatures required.

Low`active_flag` is stored in different waysOpen

`pending_queries` element has the one bit flag at the beginning. If it is set to 0, the query was already "executed". Otherwise the flag is 1 and other fields are packed in the structure. In `update_pending_queries()` this flag is saved with `store_uint()` and `store_int()`. In `message_signed_by_id?()`, `messages_by_mask()` and `unpack_query_data()` it is loaded by `load_int()`.

Low`end_parse()` Is MissingOpen

`end_parse()` checks if slice is empty, otherwise throws an exception. It allows to ensure the slice has the expected data structure.

Low`get_messages_unsigned_by_id()` name is misleadinOpen

According to the name the function `get_messages_unsigned_by_id()` should return the messages that are yet unsigned by the owner with `id`. However, the function returns the messages that were signed by that owner. `get_messages_unsigned()` returns all the messages not yet "executed".

Low`load_text_hex_number()` should check the input dOpen

`load_text_hex_number()` doesn't perform some safety checks: 1. It is not checked that `comment.slice_bits()` is divisible by 8 (extra bits are ignored). 2. It is not checked that the result fits into `int` (overflow exception is thrown). 3. It is not checked that `comment` has enough data (underflow exception is thrown).

Low`require()` can be usedOpen

The expression `if (!condition) revert(message)` can be rewritten in more common form `require(condition, message)`.

LowDeclaration Naming ConventionOpen

Some function names do not conform to the [Solidity style guide](https://solidity.readthedocs.io/en/v0.7.3/style-guide.html#naming-conventions) with regards to its naming convention.

LowEthereum bridge `config_param(71)` is hardcodedOpen

`config_param(71)` stands for Ethereum mainnet bridge. To deploy the bridge for another chain one has to edit the code. This can lead to a confusion. Also, `multisig-code` and `bridge_code` use independent code to access the `config_param` which increases the code complexity.

Show 5 more

LowFunction Should Be Declared ExternalOpen

`public` functions which are never called internally within the contract should have `external` visibility for gas optimization. Unchanged `memory` arguments of `external` functions should be declared `calldata`.

LowImproper Usage of `impure` ModifierOpen

The function `get_bridge_config()` is defined as `impure`, but it does not modify the storage or throw an exception.

LowMissing event emittingOpen

`allowBurn` field is changed by `voteForSwitchBurn()` but no event is emitted.

LowThe code can be simplifiedOpen

```cpp=15 throw_if(301, wc + 1); ``` The code can be rewritten in a more explicit way: ```cpp=15 throw_unless(301, wc == - 1); ``` ```cpp=67 int should_check = cnt_bits != old_cnt_bits; cnt -= should_check; ``` The code can be rewritten in a more explicit way: ```cpp=67 if (cnt_bits != old_cnt_bits) { cnt += 1; } ``` ```cpp=167 last_cleaned -= last_cleaned == 0; ``` The code can be rewritten in a more explicit way: ```cpp=167 if (last_cleaned == 0) { last_cleaned = 1; } ```

LowUsage of Magic NumbersOpen

There are many magic numbers used directly in code.