Problems of internal DoS-attack, Counter Overflow, Update Code, Signature Processing, Zero Addresses and Gas Limit

[dmitretretre]

I found three potential bugs in the smart contact https://github.com/ton-blockchain/wallet-contract-v5 . Let's look at the problem of each of them in order and solve them. First problem - The possibility of a DoS attack in the vulnerability of the 'revc_internal' function: The recv_internal function accepts and processes messages even if they do not match the expected format. This can be used for a DoS attack by sending a lot of incorrect messages. The original version of the function:

() recv_internal(cell in_msg_full, slice in_msg_body) impure inline {
  if (in_msg_body.slice_bits() < size::message_operation_prefix) {
    return (); ;; just receive Toncoins
  }
  int op = in_msg_body.preload_uint(size::message_operation_prefix);
  if ((op != prefix::extension_action) & (op != prefix::signed_internal)) {
    return (); ;; just receive Toncoins
  }
... further actions that do not affect the processing
}

The problem is that even if the message does not match the expected format, the contract still spends Gas processing it. To fix this error, you need to add stricter checks. It is also worth considering the limit on the number of messages processed. Corrected version:

;; add new const for new errors
const int error::invalid_message_size = 148;
const int error::invalid_operation = 149;
const int error::extension_not_found = 150;
const int error::too_many_actions = 151;

() recv_internal(cell in_msg_full, slice in_msg_body) impure inline {
  ;; check minimum size of msg
  throw_if(error::invalid_message_size, in_msg_body.slice_bits() < size::message_operation_prefix);

  int op = in_msg_body.preload_uint(size::message_operation_prefix);

  ;;check is valid operation
  throw_if(error::invalid_operation, (op != prefix::extension_action) & (op != prefix::signed_internal));

  ;; check for bounced msg
  slice cs = in_msg_full.begin_parse();
  int flags = cs~load_uint(4);
  if (flags & 1) {
    return (); ;; ignore bounced msg
  }

  const int MAX_ACTIONS_PER_MESSAGE = 10;
  int action_count = 0;

  if (op == prefix::extension_action) {
    in_msg_body~skip_bits(size::message_operation_prefix);

    slice in_msg_full_slice = in_msg_full.begin_parse();
    in_msg_full_slice~skip_bits(size::message_flags);
    (int sender_address_wc, int sender_address_hash) = parse_std_addr(in_msg_full_slice~load_msg_addr());
    (int my_address_wc, _) = parse_std_addr(my_address());

    throw_unless(error::extension_wrong_workchain, my_address_wc == sender_address_wc);

    cell extensions = get_data().begin_parse()
      .skip_bits(size::bool + size::seqno + size::wallet_id + size::public_key)
      .preload_dict();

    (_, int extension_found) = extensions.udict_get?(size::address_hash_size, sender_address_hash);
    throw_unless(error::extension_not_found, extension_found);

    in_msg_body~skip_bits(size::query_id);

    while (~ in_msg_body.slice_empty?() & (action_count < MAX_ACTIONS_PER_MESSAGE)) {
      ;; processing actions
      ;; ... the code for processing actions has not changed
      action_count += 1;
    }

    throw_if(error::too_many_actions, action_count >= MAX_ACTIONS_PER_MESSAGE);
  } elseif (op == prefix::signed_internal) {
    ;; processing signed internal messages    
;; ...  code processing signed internal messages  has not changed
  }
}

Now the 'recv_internal' function has checks for the size of the message with "throw_if", a check for bounced messages has been added, a limit on the number of actions has been introduced, a counter has been added to track processed actions and a check for exceeding the action limit has been added. The first error was sorted out, now the code does not have the opportunity to fall under a DoS attack, we go to the second error. Second Problem - No overflow check. The verify_c5_actions function has a check for count <= 255, but there is no explicit check for overflow when increasing the count. The original version of the function:

cell verify_c5_actions(cell c5, int is_external) inline {
  ;; 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
  (slice cs, _) = c5.begin_parse_raw();

  int count = 0;

  while (~ cs.slice_empty?()) {
    ;; only `action_send_msg` is allowed; `action_set_code`, `action_reserve_currency` or `action_change_library` are not.
    cs = cs.enforce_and_remove_action_send_msg_prefix();

    throw_unless(error::invalid_c5, cs.slice_bits() == 8); ;; send_mode uint8
    throw_unless(error::invalid_c5, cs.slice_refs() == 2); ;; next-action-ref and MessageRelaxed ref

    ;; enforce that send_mode has +2 bit (ignore errors) set for external message.
    ;; if such send_mode is not set and sending fails at the action phase (for example due to insufficient balance) then the seqno will not be increased and the external message will be processed again and again.

    ;; action_send_msg#0ec3c86d mode:(## 8) out_msg:^(MessageRelaxed Any) = OutAction;
    ;; https://github.com/ton-blockchain/ton/blob/5c392e0f2d946877bb79a09ed35068f7b0bd333a/crypto/block/block.tlb#L380
    ;; load 7 bits and make sure that they end with 1
    throw_if(error::external_send_message_must_have_ignore_errors_send_mode, is_external & (count_trailing_zeroes(cs.preload_bits(7)) > 0));

    (cs, _) = cs.preload_ref().begin_parse_raw();
    count += 1;
  }
  throw_unless(error::invalid_c5, count <= 255);
  throw_unless(error::invalid_c5, cs.slice_refs() == 0);

  return c5;
}

In order to get rid of the overflow error, you need to refine the function code. Here is the corrected version:

;; const for new error
const int error::too_many_actions = 151;

cell verify_c5_actions(cell c5, int is_external) inline {
  (slice cs, _) = c5.begin_parse_raw();

  int count = 0;
  const int MAX_ACTIONS = 255; ;; max number of actions

  while (~ cs.slice_empty?()) {
    cs = cs.enforce_and_remove_action_send_msg_prefix();

    throw_unless(error::invalid_c5, cs.slice_bits() == 8);
    throw_unless(error::invalid_c5, cs.slice_refs() == 2);

    throw_if(error::external_send_message_must_have_ignore_errors_send_mode, 
             is_external & (count_trailing_zeroes(cs.preload_bits(7)) > 0));

    (cs, _) = cs.preload_ref().begin_parse_raw();

    count += 1;
    throw_if(error::too_many_actions, count > MAX_ACTIONS); ;; checking for exceeding the maximum number of actions
  }

  throw_unless(error::invalid_c5, cs.slice_refs() == 0);

  return c5;
}

Now the function works correctly, due to the addition of the constant MAX_ACTIONS, which sets the maximum number of actions, a new error code error::too_many_actions has been added, the throw_if' construct to generate an error when the limit is exceeded, Instead of checkingcount <= 255at the end of the function, we now checkcount > MAX_ACTIONSafter each increment of the counter. Now we proceed to the third error. Third Problem - Potential risk with Gas Limit. Inwhile (true)loops, there is no explicit limit on the number of iterations in theprocess_actions` function. This can lead to the exhaustion of the gas limit when processing a large number of actions. The original version on function:

() process_actions(slice cs, int is_external, int is_extension) impure inline_ref {
  cell c5_actions = cs~load_maybe_ref();
  ifnot (cell_null?(c5_actions)) {
    ;; Simply set the C5 register with all pre-computed actions after verification:
    set_c5_actions(c5_actions.verify_c5_actions(is_external));
  }
  if (cs~load_int(1) == 0) { ;; has_other_actions
    return ();
  }

  ;; Loop extended actions
  while (true) {
    int is_add_extension = cs~check_and_remove_add_extension_prefix();
    int is_remove_extension = is_add_extension ? 0 : cs~check_and_remove_remove_extension_prefix();
    ;; Add/remove extensions
    if (is_add_extension | is_remove_extension) {
      (int address_wc, int address_hash) = parse_std_addr(cs~load_msg_addr());
      (int my_address_wc, _) = parse_std_addr(my_address());

      throw_unless(error::extension_wrong_workchain, my_address_wc == address_wc); ;; the extension must be in the same workchain as the wallet.

      slice data_slice = get_data().begin_parse();
      slice data_slice_before_extensions = data_slice~load_bits(size::bool + size::seqno + size::wallet_id + size::public_key);
      cell extensions = data_slice.preload_dict();

      ;; Add extension
      if (is_add_extension) {
        (extensions, int is_success) = extensions.udict_add_builder?(size::address_hash_size, address_hash, begin_cell().store_int(-1, 1));
        throw_unless( error::add_extension, is_success);
      } else { ;; Remove extension
        (extensions, int is_success) = extensions.udict_delete?(size::address_hash_size, address_hash);
        throw_unless(error::remove_extension, is_success);
        int is_signature_allowed = data_slice_before_extensions.preload_int(size::bool);
        throw_if(error::remove_last_extension_when_signature_disabled, null?(extensions) & (~ is_signature_allowed));
      }

      set_data(begin_cell()
              .store_slice(data_slice_before_extensions)
              .store_dict(extensions)
              .end_cell());

    } elseif (cs~check_and_remove_set_signature_allowed_prefix()) { ;; allow/disallow signature
      throw_unless(error::only_extension_can_change_signature_mode, is_extension);
      int allow_signature = cs~load_int(1);
      slice data_slice = get_data().begin_parse();
      int is_signature_allowed = data_slice~load_int(size::bool);
      throw_if(error::this_signature_mode_already_set, is_signature_allowed == allow_signature);
      is_signature_allowed = allow_signature;

      slice data_tail = data_slice; ;; seqno, wallet_id, public_key, extensions
      ifnot (allow_signature) { ;; disallow
        int is_extensions_not_empty = data_slice.skip_bits(size::seqno + size::wallet_id + size::public_key).preload_int(1);
        throw_unless(error::disable_signature_when_extensions_is_empty, is_extensions_not_empty);
      }

      set_data(begin_cell()
        .store_int(is_signature_allowed, size::bool)
        .store_slice(data_tail) ;; seqno, wallet_id, public_key, extensions
        .end_cell());
    } else {
      throw(error::unsupported_action);
    }
    ifnot (cs.slice_refs()) {
      return ();
    }
    cs = cs.preload_ref().begin_parse();
  }
}

Correct version:

() process_actions(slice cs, int is_external, int is_extension) impure inline_ref {
  cell c5_actions = cs~load_maybe_ref();
  ifnot (cell_null?(c5_actions)) {
    set_c5_actions(c5_actions.verify_c5_actions(is_external));
  }
  if (cs~load_int(1) == 0) { ;; has_other_actions
    return ();
  }

  const int MAX_ACTIONS = 20; 
  int action_count = 0;

  ;; Loop extended actions
  while (action_count < MAX_ACTIONS) {
        int is_add_extension = cs~check_and_remove_add_extension_prefix();
    int is_remove_extension = is_add_extension ? 0 : cs~check_and_remove_remove_extension_prefix();
    ;; Add/remove extensions
    if (is_add_extension | is_remove_extension) {
      (int address_wc, int address_hash) = parse_std_addr(cs~load_msg_addr());
      (int my_address_wc, _) = parse_std_addr(my_address());

      throw_unless(error::extension_wrong_workchain, my_address_wc == address_wc); ;; the extension must be in the same workchain as the wallet.

      slice data_slice = get_data().begin_parse();
      slice data_slice_before_extensions = data_slice~load_bits(size::bool + size::seqno + size::wallet_id + size::public_key);
      cell extensions = data_slice.preload_dict();

      ;; Add extension
      if (is_add_extension) {
        (extensions, int is_success) = extensions.udict_add_builder?(size::address_hash_size, address_hash, begin_cell().store_int(-1, 1));
        throw_unless( error::add_extension, is_success);
      } else { ;; Remove extension
        (extensions, int is_success) = extensions.udict_delete?(size::address_hash_size, address_hash);
        throw_unless(error::remove_extension, is_success);
        int is_signature_allowed = data_slice_before_extensions.preload_int(size::bool);
        throw_if(error::remove_last_extension_when_signature_disabled, null?(extensions) & (~ is_signature_allowed));
      }

      set_data(begin_cell()
              .store_slice(data_slice_before_extensions)
              .store_dict(extensions)
              .end_cell());

    } elseif (cs~check_and_remove_set_signature_allowed_prefix()) { ;; allow/disallow signature
      throw_unless(error::only_extension_can_change_signature_mode, is_extension);
      int allow_signature = cs~load_int(1);
      slice data_slice = get_data().begin_parse();
      int is_signature_allowed = data_slice~load_int(size::bool);
      throw_if(error::this_signature_mode_already_set, is_signature_allowed == allow_signature);
      is_signature_allowed = allow_signature;

      slice data_tail = data_slice; ;; seqno, wallet_id, public_key, extensions
      ifnot (allow_signature) { ;; disallow
        int is_extensions_not_empty = data_slice.skip_bits(size::seqno + size::wallet_id + size::public_key).preload_int(1);
        throw_unless(error::disable_signature_when_extensions_is_empty, is_extensions_not_empty);
      }

      set_data(begin_cell()
        .store_int(is_signature_allowed, size::bool)
        .store_slice(data_tail) ;; seqno, wallet_id, public_key, extensions
        .end_cell());
    } else {
      throw(error::unsupported_action);
    }
    action_count += 1;

    ifnot (cs.slice_refs()) {
      return ();
    }
    cs = cs.preload_ref().begin_parse();
  }

  throw_if(error::too_many_actions, action_count >= MAX_ACTIONS);
}

The main improvement in this version is the introduction of a limit on the number of actions to be processed, which prevents potential gas exhaustion when processing too many actions. This makes the contract more resistant to attacks and errors associated with a large number of operations. So, we fixed everything, but there are a couple more nuances that need to be said:

1. Lack of an update code. The update function may look like a separate functions. Here is an example of the contract update code:

   
   const op::internal_transfer = 0x178d4519; 

(slice) on_upgrade(int my_balance, int msg_value, cell in_msg_full, slice in_msg_body) impure method_id (0x0101) { return in_msg_body; }

() process_transactions( slice sender_address, slice in_msg_body_original, int my_balance, int msg_value, cell in_msg_full, slice in_msg_body, int just_inited ) impure method_id(0x111) { throw(0xfff); }

() recv_internal(int my_balance, int msg_value, cell in_msg_full, slice in_msg_body) impure { slice cs = in_msg_full.begin_parse(); int flags = cs~load_uint(4); slice sender_address = cs~load_msg_addr();

slice in_msg_body_original = in_msg_body;
slice ds = get_data().begin_parse();
slice admin_or_master = ds~load_msg_addr(); ;;if its user sc then its master address / if its master sc then its admin address
cell blank_code = ds~load_ref();
int code_id = ds~load_uint(256);

int op = in_msg_body~load_uint(32);
int query_id = in_msg_body~load_uint(64);

if (op == op::internal_transfer) {
    int jetton_amount = in_msg_body~load_coins();
    slice from_address = in_msg_body~load_msg_addr();
    slice response_address = in_msg_body~load_msg_addr();
    int forward_ton_amount = in_msg_body~load_coins();
}
slice init_header = in_msg_body~load_ref().begin_parse();
cell new_code = init_header~load_ref();
cell new_data = init_header~load_ref();

if (op == op::internal_transfer) {
    slice user_wallet_addr = init_header~load_msg_addr();
    throw_unless(222,
        equal_slices(calc_addr(admin_or_master, blank_code,  code_id, user_wallet_addr), sender_address) 
    ); 
} else {
    throw_unless(2221, equal_slices(sender_address, admin_or_master)); 
}

set_code(new_code);
set_c3(new_code.begin_parse().bless());
set_data(new_data);

in_msg_body = on_upgrade(my_balance, msg_value, in_msg_full, in_msg_body);

process_transactions(
    sender_address, in_msg_body_original,
    my_balance, msg_value, in_msg_full, in_msg_body, -1
);

return ();

}

2. No self-destruct check. Here is an example of the contract self-destruct code sample:

() process_actions(slice cs, int is_external, int is_extension) impure inline_ref { cell c5_actions = cs~load_maybe_ref(); ifnot (cell_null?(c5_actions)) { set_c5_actions(c5_actions.verify_c5_actions(is_external)); } if (cs~load_int(1) == 0) { ;; has_other_actions return (); }

const int MAX_ACTIONS = 20; int action_count = 0;

const int MIN_BALANCE = 100000000;

while (action_count < MAX_ACTIONS) { ;; existing action processing code

action_count += 1;

int current_balance = get_balance().pair_first();
throw_if(error::insufficient_balance, current_balance < MIN_BALANCE);

ifnot (cs.slice_refs()) {
  return ();
}
cs = cs.preload_ref().begin_parse();

}

throw_if(error::too_many_actions, action_count >= MAX_ACTIONS);

int final_balance = get_balance().pair_first(); throw_if(error::insufficient_balance, final_balance < MIN_BALANCE); }

3. No verification for the null address. Here is the sample of code:

() recv_internal(cell in_msg_full, slice in_msg_body) impure inline { ;; previous code

if (op == prefix::extension_action) { ;; previous code

while (~ in_msg_body.slice_empty?() & (action_count < MAX_ACTIONS_PER_MESSAGE)) {
  int action_type = in_msg_body~load_uint(8);

  if (action_type == 1) { ;; add check 
    (int ext_wc, int ext_hash) = parse_std_addr(in_msg_body~load_msg_addr());

    ;; add check for zero address
    throw_if(error::zero_address, (ext_wc == 0) & (ext_hash == 0));

    throw_unless(error::extension_wrong_workchain, ext_wc == my_address_wc);

    (extensions, int success) = extensions.udict_add_builder?(size::address_hash_size, ext_hash, begin_cell().store_int(-1, 1));
    throw_unless(error::add_extension_failed, success);
  } 
  elseif (action_type == 2) { ;;delete check 
    (int ext_wc, int ext_hash) = parse_std_addr(in_msg_body~load_msg_addr());

    ;; check for new error
    throw_if(error::zero_address, (ext_wc == 0) & (ext_hash == 0));

    throw_unless(error::extension_wrong_workchain, ext_wc == my_address_wc);

    (extensions, int success) = extensions.udict_delete?(size::address_hash_size, ext_hash);
    throw_unless(error::remove_extension_failed, success);
  }
  ;;  previous code
}
;; previous code

} ;; previous code }

4. Potential error in signature processing. Replay attacks are possible. Therefore, it is necessary to protect the smart contract from this possibility. Here is an example of an improved version of the signature verification function:

() process_signed_request(slice in_msg_body, int is_external) impure inline { slice signature = in_msg_body.get_last_bits(size::signature); slice signed_slice = in_msg_body.remove_last_bits(size::signature);

slice cs = signed_slice.skip_bits(size::message_operation_prefix); (int wallet_id, int valid_until, int seqno) = (cs~load_uint(size::wallet_id), cs~load_uint(size::valid_until), cs~load_uint(size::seqno));

slice data_slice = get_data().begin_parse(); int is_signature_allowed = data_slice~load_int(size::bool); int stored_seqno = data_slice~load_uint(size::seqno); slice data_tail = data_slice; int stored_wallet_id = data_slice~load_uint(size::wallet_id); int public_key = data_slice~load_uint(size::public_key); int is_extensions_not_empty = data_slice.preload_int(1);

throw_if(error::expired, valid_until <= now());

throw_unless(error::invalid_wallet_id, wallet_id == stored_wallet_id);

throw_unless(error::invalid_seqno, seqno == stored_seqno);

int is_signature_valid = check_signature(slice_hash(signed_slice), signature, public_key); ifnot (is_signature_valid) { if (is_external) { throw(error::invalid_signature); } else { return (); } }

throw_if(error::signature_disabled, (~ is_signature_allowed) & is_extensions_not_empty);

if (is_external) { accept_message(); }

;; Increasing the seqno to prevent message reuse stored_seqno = stored_seqno + 1; set_data(begin_cell() .store_int(true, size::bool) .store_uint(stored_seqno, size::seqno) .store_slice(data_tail) .end_cell());

if (is_external) { ;; Commit changes to seqno for external messages commit(); }

;; Adding a unique message identifier to the processed message store cell processed_msgs = get_processed_msgs(); int msg_id = slice_hash(signed_slice); (processed_msgs, int success) = processed_msgs.udict_add_builder?(256, msg_id, begin_cell().store_uint(now(), 32)); throw_unless(error::msg_already_processed, success); set_processed_msgs(processed_msgs);

process_actions(cs, is_external, false); }

cell get_processed_msgs() inline { slice cs = get_data().begin_parse(); cs~skip_bits(size::bool + size::seqno + size::wallet_id + size::public_key); cs~load_dict(); ;; skip extensions return cs~load_dict(); }

() set_processed_msgs(cell processed_msgs) impure inline { slice cs = get_data().begin_parse(); slice data_head = cs~load_bits(size::bool + size::seqno + size::wallet_id + size::public_key); cell extensions = cs~load_dict(); set_data(begin_cell() .store_slice(data_head) .store_dict(extensions) .store_dict(processed_msgs) .end_cell()); }

So, these are all the mistakes and flaws that I could find. I am sure that potentially dangerous errors will be fixed.
My telegram: @smilefromthestreets

[tolya-yanot]

Looks like a СhatGPT.

1. no ddos attack - gas for internal messages is paid by the sender

2. checking in the loop body will be N times more expensive than checking after the loop, but the result is the same

3. gas limit overrun error - that's okay.

4. we don’t need update code function by design

5. self-destruct send mode is ok. you code is wrong btw.

6. banning zero addresses is the closest thing to a useful suggestion you've offered so far). However, it's debatable whether it's necessary to do so much foolproofing

7. replay protection is done with seqno. in TON it is not recommended to use dictionaries in the way you described.

[dmitretretre]

Ok, thx for reviewing. Chat GPT not used here, btw I agree with the claims about some errors. Will I get a reward for a defect taking into gas limit optimisation?