Info

Download ZIP (771.8 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

29

Comments

Сontinuation about Keen Fox Issue
## transaction-checker
Yes, you are right about the unfinished script. There just wasn't enough time. But as I wrote above, the validator check will remain valid in this case (see above).  We do not need to check the weight again if it is checked on the block verification side, here we only check the validity of the validators themselves relative to the block smart-contract. 
Yes, the remark about impure is true, but the Fift compiler took into account verify_signatures and verify_block_shard_data.
You have not added any comments yet...
by rating

Issues

3. Checking in the scripts alone does not eliminate the need for verification on-chain! Relying solely on off-chain checks leaves the system vulnerable to attacks.
macOS
2
Sacred Hare Feb 7 at 14:46
Yes, I agree that the verification should be in the smart contract itself. But again, the script interacts on-chain, Since it has a native network connection without interacting off-chain.
## lite-client

`check_signatures` functions uses only `total_weight` from actual config34. The code extracts pubkeys, signatures and even data itself from `signatures` parameter that could be totally artificial.

So it looks like both `check_block` and `new_key_block` messages don't check provided block for correctness.

## transaction-checker

Script preparing `check_transaction` message for transaction-checker doesn't look finished.

Again looks like nothing stops me from preparing `check_transaction` with fake key-block signatures and using them to sign fake transaction.

Also some important functions in `checker.fc` (like `verify_signatures`, `verify_block_shard_data`, etc) aren't marked `impure` so I believe they are missing in compiled smc bytecode.

In `verify_signatures` function there is only number of valid signatures considered, not their weight.
1
Sacred Hare Feb 5 at 13:29
Thanks for issue!
## lite-client
I agree with the vulnerability that has arisen on the side of the smart-contract. But this is not available on the script side, since data about the validator and its weight are taken from real network configs, since Pytoniq is a Native ADNL with a direct connection to the LiteServer. In addition, it can be noted that the signatures are correctly verified by a real validator:
- node_id_short is calculated for each validator from the official list.
- A signature is accepted only if it matches an existing validator.
And validator weight take from real config. In this case, all checks (new_key_block and check_block) are fully valid.Therefore, the risk of tampering with validator weights is reduced to 0. If something wasn't clear, you can take another look Python script 208-259.
The website says that the text is too big, and I can't double reply)
Hey, good work with the bridge. Some areas need extra attention:

1. The lite-client and transaction-checker contracts seem unnecessarily coupled. Block checking is done in the transaction-checker instead of the lite-client, forcing you to send validator sets with your message and causing gas inefficiencies.

2. Missing header proof verification in new_key_block operations and an incomplete one for check_transaction.

3. Inconsistent signature verification methods between the lite-client and checker (weight-based vs. count-based).

What do you think?
macOS
Sacred Hare Feb 6 at 23:20
Hey, thanks for issue!
1. The blocks are checked on the side of the LiteClient. At the Checker, they undergo another inspection before making further checks on transaction_proof.
2. Yes, that's true. We didn't have time to finish this check on smart-contract side. But there is a block_header_proof checking on the script side.
3. I've already answered this question, check out the issue above. I've already answered this question, check out the issue above. In short, we don't need to fully look at the information about the validator in the smart contract, because we have full native verification on the script side and signature forgery is impossible, since the script interacts directly with real network configurations.
There doesn't seem to be a check to verify that the root_hash within the cryptographic message (for signature checking) relates to the root_hash of the given block.

There also doesn't seem to be protection from double-counting a validator's weight on duplicate signatures. (mine had the same issue).

The code looks clean though
Sacred Hare Feb 7 at 14:53
Thanks for issue!
Yes, I already answered the question about block_header_proof. In script we use this struct `b'pn\x0b\xc5' + mc_block(or block_id_ext)[0].root_hash + mc_block[(or block_id_ext)0].file_hash` We wanted to add this check, but we didn't have enough time to include it in the script.
To bypass the double-counting verification, we use the verification of each validator in turn. But I agree that this is not the best solution. Again, it all comes down to time :(
In lite-client contract in `process_new_key_block` function you use validators from new `block`. You should take validators from the lite-client contract state.
Sacred Hare Feb 9 at 01:32
Yes, this is an important point, but reread the code and script again. Only after the signatures have been verified by the current validators can the set of validators be updated.
In lite-client contract in `new_key_block` function:
1) you do not check that new block is key and master block
2) you do not check that `prev_key_block_seqno` from new block is equal to last known seqno

In `check_signatures` function you don't check that some validator is used several times. You should check duplicates.

In function `check_block` in lite-client contract you don't check that the block in from currently known epoch. In current epoch currently known validators have power. In prev/next epoch currently known validators may have no majority.
Sacred Hare Feb 9 at 01:48
1) Yes, it makes sense, but it's not necessary, as any block not in mс will fall.
2) We check it: `throw_unless(error::invalid_seqno, block_seqno > known_block_seqno)`.
3) Each validator is checked in turn after the other, this prevents the risk of identical validators being used. But I agree that this should be added to the verification.
4) I agree here, I don't understand how I didn't see it before. On the script side, we've done epoch-based processing.In any case, we check the last_seqno, and this does not give the moment of forgery.
Did you test your Transaction-checker contract? Anybody can call `check_transaction` function. But you check that sender is lite client.
I did not found code that checks that transaction is present in tx proof. You have `verify_transaction_proof` function. But it's weird code.
Sacred Hare Feb 9 at 01:54
Yes, I tested with hardcode data, as there was no time to add normal processing. Everything is working as it should. As for verifying the transaction, just look at the code again. Anyone can send a verification request, the lite-client address is used as the relationship between the contracts.
In block checks both keys and signatures are from incoming message, thus can be forged. In transaction checker, proof checks are generated by LLM and has no sense (check_merkle_proof in particular). Basically contracts implement partial parsing of data structures in accordance to block.tlb and no cryptographic checks with sense.
in signature check, there is no check between to_sign and provided "block" hash (block_hash is unused in extract_validators_from_block),the same applies for check_block, there only seqno is checked from the "block", and signatures are checked separately, check_signatures does not do ANY matching between validators_slice and signatures_dict, only total_weight is used from validators_slice

More info on issues: https://contest.com/docs/TrustlessBridgeChallengeAssessment
Sacred Hare Feb 20 at 01:47
Thanks for issue! We didn't use any LLM generators. check_merkle_proof is a mistake on my part, which I haven't had time to fix. Yes, we could add more checks, let's say a full check, and not only_weight check, but alas - everything rested on time :(
1) In extract_validators_from_block, we can checked block_slice, if block_slice is failed / invalid, then block_hash is failed.
2) No, we сan checked Signatures and seqno. But yes, load only block_seqno params. If we know trust seqno -> signatures, then send_response.
3) If weight is true, then validators_slice match to signatures_dict. Also we can checked pubkey from validator.
I agree that this code needs to be updated, but errors are not considered problematic, I do not see it possible to compare all the necessary parameters for hacking.
Nobody added any issues yet...