Info

Fluffy Lemur
Author
Download ZIP (490.4 KB)

Testing and Issues

You can test this entry and submit issues during the testing period of the TON Trustless Bridge Challenge contest.

Entries with serious issues will not be able to win the contest, but even minor issues might be important for overall results.

Voting

55
by rating

Issues

Subtle Swallow Feb 7 at 00:30
In `new_key_block` function only signatures are checked. There are no checks for `seqno` from new block. We must check:
- `seqno` from new block is greater than last known seqno
- `prev_key_block_seqno` from new block is equal to known seqno.

But in solution there are no checks. It means that we can send mc blocks from current epoch in any order and `lite-client` contract will accept that blocks.
3
Fluffy Lemur Feb 7 at 18:03
According to the contest requirements, the only check for a new key block is that it “corresponds to the currently known epoch.”

Since a block signed by the current validator set implicitly confirms it belongs to the current epoch, adding seqno checks isn’t mandated.

It explicitly allows key blocks within an epoch to be accepted in any order. The epoch only changes when a key block carrying a config update (and signed by the current validator set) is received. Therefore, additional ordering checks based on seqno would be an unnecessary overhead.
Subtle Swallow Feb 7 at 00:17
Error in `check_block_signatures` function. Weight of signatures must be STRICTLY more than 2/3 of total weight.
In your code:
```
return signed_weight * 3 >= 2 * g::total_weight;
```

In original TON code
https://github.com/ton-blockchain/ton/blob/master/crypto/block/check-proof.cpp#L659

So, if `signed_weight * 3 == 2 * total_weight`, than is bad block. But according your code it's valid block.
2
Fluffy Lemur Feb 7 at 18:05
Agreed—the signature weight must be strictly greater than 2/3 of the total. While the chance of exactly hitting the equality case (signed_weight * 3 == 2 * total_weight) is extremely low, for correctness we should enforce a strict inequality.
Keen Fox Feb 8 at 03:49
Hey! Very nice solution. Code is clean and easy to follow.

Lite-client uses some extension to `check_block` and `correct` messages to pass some "callback" data. It's really nice and simple approach, but not sure if it's allowed by the task.

Tx-checker accepts all bounced messages. So if associated lite-client fails to check a block, coins will be accumulated on tx-checker smc address. Would be nice to have explicit handler for bounced messages to return excesses to the original `check_transaction` sender.
2
Subtle Swallow Feb 7 at 00:40
In `check_block` function there is no check that the block corresponds to currently known epoch. It means that your code should checks:
- `prev_key_block_seqno from testing block` >= `first key block seqno in epoch`
- `prev_key_block_seqno from testing block` >= `last known key block seqno`
1
Fluffy Lemur Feb 7 at 18:06
These additional checks are redundant. It’s sufficient to verify that the block is signed by the current set of validators—successful signature verification implies that the block belongs to the current epoch. Since the contest doesn’t define an “epoch” explicitly, we consider an epoch to be determined by the current validator set. When the validator set changes, so does the epoch. Thus, if a lite-client submits a block from a previous epoch, the signature check will fail.
Subtle Swallow Feb 7 at 01:06
Bug in `tx_checker` contract in `op_handler::check_transaction` function.
1) You don not check that hash of `prof` cell is equal to hash of block that is sent to `lite-client` contract.

2) `throw_unless(err::tx_hash_mismatch, tx.cell_hash() == tx_hash);` It is does not work if `tx` is pruned cell. Use `0 CHASHI` opcode
1
Fluffy Lemur Feb 7 at 18:10
1. Agreed—the missing check for the hash of the proof cell is a mistake; it was likely removed accidentally during a merge or forgotten...

2. Regarding the second point, since the contest did not precisely define the input data format, we assumed that the complete transaction (without any pruned cells) would be provided.
Subtle Swallow Feb 7 at 00:45
You should send rest in inbound message op::new_key_block::success and op::new_key_block::success messages
Fluffy Lemur Feb 7 at 18:14
To be honest, we didn’t quite understand what exactly was meant, can you explain please?
Subtle Swallow Feb 7 at 22:23
More comments about my comment published at `Feb 7 at 00:45`. You store rest of `in_msg_body` in output action. `in_msg_body` can be not empty. In common case we should check that `in_msg_body` is empty after parsing message.
Subtle Swallow Feb 7 at 23:20
Typo in my comment  `Feb 7 at 00:40`:
- `prev_key_block_seqno from testing block` <= `last known key block seqno`
Mellow Squid Feb 19 at 11:15
Solid implementation of lite-client. Max_main_validators weight logic is implemented, but "only main validators can sign" isn't. No prev_keyblock/global_id checks (minor). Proper implementation of searching for tx in the block, but missed check of correspondence proof and block that sent to LC. Quite optimized gas usage.

More info on issues: https://contest.com/docs/TrustlessBridgeChallengeAssessment
Fluffy Lemur Feb 19 at 12:22
Thank you very much for the review.

"only main validators can sign" isn't — is an incorrect assertion

https://github.com/RSquad/trustless-bridge-contracts/blob/0b01bd8288f3de833e1ec89d8be1a8c0e836db27/contracts/lite_client/storage.fc#L23

Here we write to the state only the main validators from config34 of the key block, then use only validators in state to check signatures
Mellow Squid Feb 19 at 12:30
Indeed, once validators are filtered to leave only main ones before saving, it is safe to accept signatures from any validator from filtered set. Accepted.
Nobody added any issues yet...