Info

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

37

Comments

At submission, I was not aware of XCTOS and used full blocks for the implementation as workaround. Another issue in my submission is that it didn't protect against duplicate signatures. I created a repo that includes fixes for both issues at https://github.com/Jaapp-/trustless-bridge-challenge-amendments
You have not added any comments yet...
by rating

Issues

From README: "this implementation uses full block data to avoid pruned branches"

"It seems to be impossible to work with proofs"


I think I saw some submission parse exotic cells manually.

You could for example use 'XCTOS' to convert exotic cell into slice and parse manually.

Instruction docs: https://docs.ton.org/v3/documentation/tvm/instructions#XCTOS

TACT snippet:

struct Struc {
sli: Slice;
flg: Bool;
}

asm fun prs(c: Cell): Struc { XCTOS }
Windows 10
1
Mad Otter Feb 7 at 05:01
Yes, you're right. I wasn't aware of this instruction at time of submission, and it crippled my implementation's efficiency. I uploaded a repo with a fix that is much more gas efficient.
Gas consumption in `findTrustedTransaction` function:
1) You iterate over all cells to find the transaction.
2) You calculate hash for slice: `slice.hash()`. Opcode HASHSU takes 526 units of gas. You should use `root.hash()`. Opcode HASHCU takes only 26 units of gas.
526 / 26 = 20 times!
1
Implemented basic functionality in Tact with some security issues: in lite-client main_validators part is not implemented, in tx-checker full cell scan is used (expensive and insecure). Funny way of parsing vaidator list.
More info on issues: https://contest.com/docs/TrustlessBridgeChallengeAssessment
1
Mad Otter Feb 20 at 06:16
Thanks for the feedback! Two points:

1. The distinction of main validators is not in the whitepaper and not implemented in the TS example linked from the challenge. Was this documented elsewhere?

2. (about tx-checker being insecure) My original submission checks for transaction$0111 and doesn't traverse deeper, so any malicious message contents would never be parsed by the transaction checker
1. Contest task implementation implies extensive research. While we neglect some issues which are either too deep or too insignificant, `max_main_validator` were mentioned in the task and this logic is considered as required.

2. Indeed, while 0b0111 check prevent from going somewhere inside tx, it doesn't make this search fully secure. Any parent cell that starts with 0b0111 prevents from search in child cells. In ShardAccountBlocks tree it may happen, for instance, if in one of HasMapAug, label will be encoded with hml_short$0 and Unary field will contain three 1's (in that case tx which happen to be deeper in the tree will be unprovable). Generally not being able to understand where exactly in block structure some data is found (and thus what it means) is a security flaw.
1
In function `parseValidatorDescMap` (that is called on new_key_block message) you parse validator set manually. You can use TVM opcodes. It's OK.
In that function you takes all validators but you should take only `main` validators and use only them in `validateSignatures` function.

https://github.com/ton-blockchain/ton/blob/master/crypto/block/mc-config.cpp#L1865
In `validateSignatures` function:
1)
```
let requiredWeight = self.validatorSet.total_weight * 2 / 3;
```
Here is rounding error. It's better to use DIVR opcode or `(self.validatorSet.total_weight * 2 + 2) / 3`.
2)
Your code:
```
return weight >= requiredWeight
```
If weight == requiredWeight than it's not ok.
https://github.com/ton-blockchain/ton/blob/master/crypto/block/check-proof.cpp#L659
Mad Otter Feb 9 at 04:07
1) Agree, I think it's better to stick with multiplication only
2) I got >= from the TON white paper which states "Even more importantly, masterchain
blocks must collect the signatures of at least two-thirds of all the validators
(by stake)"
In lite-client contract in function `check_block` you don't check that the testing block is from current epoch.
Mad Otter Feb 9 at 04:13
prev_key_block_seqno is checked at the end of validateMcBlock which is called from check_block
Nobody added any issues yet...