Potential loss of actions included in the order and incorrect counting of the number of signatories

[zpnst]

Subject: Vulnerability Report on Multisig 2.0 Smart Contract

Dear competition organizers,

So, order_body is a dictionary where the keys are indexes of some actions, and the values are the actions themselves.

The initiator of the order fills in this dictionary himself and sends the transaction to a multi-subscription wallet with the opcode op == op::new_order.

On line 120 in the multisig.func file, this dictionary is loaded from an incoming transaction from the initiator of the order.

cell order_body = in_msg_body~load_ref();

After reaching the threshold, the order contract in the try_execute() function sends a transaction to the wallet for its execution.

The execute_order() function iterates through this dictionary and performs some action at each iteration of the loop

    int action_index = -1;
    do {
        (action_index, slice action, int found?) = order_body.udict_get_next?(ACTION_INDEX_SIZE, action_index);
        if (found?) {
            action = action.preload_ref().begin_parse();
            int action_op = action~load_op();
            if (action_op == actions::send_message) {
                int mode = action~load_uint(8);
                send_raw_message(action~load_ref(), mode);
                action.end_parse();
            } elseif (action_op == actions::update_multisig_params) {
                threshold = action~load_index();
                signers = action~load_nonempty_dict();
                signers_num = validate_dictionary_sequence(signers);
                throw_unless(error::invalid_signers, signers_num >= 1);
                throw_unless(error::invalid_threshold, threshold > 0);
                throw_unless(error::invalid_threshold, threshold <= signers_num);

                proposers = action~load_dict();
                validate_dictionary_sequence(proposers);

                action.end_parse();
            }
        }
    } until (~ found?);

Problem

The udict_get_next function?() in FunC, it takes the key as the second argument and finds an entry in the dictionary, the key of which leads to the argument passed to it.

On line 27, the initial value for dictionary iteration is selected as -1.

int action_index = -1;

If the index of the action is less than -1, then it simply will not be executed...

The same problem exists in the validate_dictionary_sequence() function:

int validate_dictionary_sequence(cell dict) impure inline {
    int index = -1;
    int expected_index = 0;
    do {
        (index, slice value, int found?) = dict.udict_get_next?(INDEX_SIZE, index);
        if (found?) {
            throw_unless(error::invalid_dictionary_sequence, index == expected_index);
            expected_index += 1;
        }
    } until (~ found?);
    return expected_index;
}

The solution to the problem

I suggest defining the initial value of the action_index variable as follows:

    int action_index = udict_delete_get_min(order_body, ACTION_INDEX_SIZE);
    action_index -= 1;

Also in the validate_dictionary_sequence() function:

    int index = udict_delete_get_min(dict, INDEX_SIZE);
    index -= 1;

Why it should help

The user can interact with the TON blockchain and with smart contracts in them using different languages and APIs. Do not hope that the user will not make mistakes when forming an order, so it is better to dynamically select the minimum value for iteration through dictionaries, rather than using the -1 constant.

Therefore, you should beware of such cases, as this may cause the actions included in the order to be lost or the number of signatories may be incorrectly calculated.

Another solution to the problem

If it is required that the index of the action or signer be >= 0, then when forming an order using the opcode op::new_order, a check should be made:

        cell order_body = in_msg_body~load_ref();
        int min_action_index = udict_delete_get_min(order_body, ACTION_INDEX_SIZE);
        throw_unless(error::invalid_min_action_index, min_action_index >= 0);

Conclusion

Best regards, Taiga Labs!

[ProgramCrafter]

"u" letter in udict_get_next? stands for "unsigned', meaning that keys are interpreted as unsigned ACTION_INDEX_SIZE-sized integers; thus, there cannot be one with index less or equal to -1.

[tolya-yanot]

@ProgramCrafter is right. Dicts have uint keys