TON-Ethereum Bridge

CertiK12 Sep 2022Code version: not recorded (see the report)

Audit

CertiK reviewed XP Network (TON-Ethereum Bridge) on 12 Sep 2022: 9 findings, 0 critical; 8 marked fixed, 1 open.

No open critical findings

Open the report Open XP Network’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings9

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

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

Medium"Replay failed" attack can drain all the balanceFixed

Burner `recv_external()` works this way: 1. Signature checked 2. Message accepted 3. Raw messages sent 4. `stored_seqno` incremented According to [documentation](https://ton.org/docs/#/smart-contracts/accept?id=external-messages), if after `accept_message()` some error will be thrown (both in ComputePhase or ActionPhase) transaction will be written to blockchain and fees will be deducted from contract balance, but storage will not be updated and actions will not be applied as in any transaction with error exit code. That way, if contract accepted external message and then throw an exception due to some error in message data or due to sending wrongly serialized message, it will pay for processing but has no opportunity to prevent message replay. The same message will be accepted by contract over and over until it consumes the whole balance.

Medium`this_address` is not checked in "change public kFixed

`this_address` is loaded from incoming message, however, not compared to `my_address()`. This opens a "replay attack" vector - the signed message from the testnet or another deployment of the contract can be reused on mainnet.

Low`end_parse()` Is MissingFixed

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

Low`recv_internal()` should be refactoredFixed

`recv_internal()` function contains 300 lines and implements different functionality. A lot of code is duplicated many times.

LowNo ability to remove from "whitelist"Fixed

There is no ability to remove items from the whitelist. In case some `nft_collection` is compromised, it will never be deleted from the whitelist.

LowPull-Over-Push PatternFixed

The change of `public_key` by function "change public key" is done without guaranteeing the `new_public_key` is able to actuate transactions on-chain. For example, zero value can be assigned by mistake.

LowRedundant StatementsFixed

The linked statements do not affect the functionality of the codebase and appear to be leftovers from test code.

LowThe meaning of `action_id` is unclearFixed

The contract contains `action_id` field, it can be retrieved via `get_action_id()` get-method. The value is incremented as a reaction on `op::ownership_assigned()` and `op::excesses()`. In addition the contract accepts `action_id` as part of incoming message, it is saved in `consumed_actions`. This leads to a confusion. The meaning of the first field is unclear. It can be influenced by any third-party. The code of `op::ownership_assigned()` and `op::excesses()` processing is redundant and can be omitted.