ton-blockchain / wallet-contract-v5

w5
MIT License
69 stars 16 forks source link

Wrong XCTOS comment (not yet vulnerability) #23

Closed nns2009 closed 1 month ago

nns2009 commented 2 months ago
;; XCTOS doesn't automatically load exotic cells (unlike CTOS `begin_parse`).
;; we use it in `verify_c5_actions` because during action phase processing exotic cells in c5 won't be unfolded too.
;; exotic cell starts with 0x02, 0x03 or 0x04 so it will not pass action_send_msg prefix check

I looked into TON’s source code and found that the second line of this comment is actually wrong: exotic cells will be unfolded (and here is a link to load_cell_slice function). Upon further inspection, this discrepancy doesn’t seem to lead to any vulnerability, because current valid exotic cell prefixes won’t pass 0ec3c86d enforce_and_remove_action_send_msg_prefix verification, and if we tried to construct an exotic cell, which starts with 0ec3c86d to pass this check, it would not pass the current exotic cell unfolding process … that is at least until an exotic cell of type 0x0E gets introduced - depending on how it unfolds, this may lead to potential vulnerabilities of Wallet v5 in the future.

Suggested fix

Avoid discrepancy, use CTOS = normal begin_parse. Or at least fix the comment.

EmelyanenkoK commented 1 month ago

exotic cells will be unfolded (and here is a link to load_cell_slice function).

If exotic cell will be passed to load_cell_slice it will cause exception, because it passes can_be_special=false into load_cell_slice_impl.

e6654321 commented 1 month ago

This will introduce another problem when passing exotic cells on external calls and cause an exception. Essentially exotic cells will lose meaning when passed externally

EmelyanenkoK commented 1 month ago

Actually no. The reason why we use XCTOS instead of CTOS is as following: in TVM CTOS under the hood can make "high-level" thing: in particular if it gets library cell for opening, it will substitute cell with library content found in global context for corresponding masterchain block.

However, when node works with c5, action list register, it doesn't implement such logic, instead it simply try to open this cell and throw exception if this cell is exotic. We repeat this behavior in c5 check in TVM by using more "low-level" instruction XCTOS that doesn't implement substitution logic.