ton-blockchain / multisig-contract-v2

Multiowner wallet
54 stars 19 forks source link

No proposers & cleaning stage (safe for beginner users) #35

Closed SC-One closed 6 months ago

SC-One commented 6 months ago

no proposers | common addresses(extra fees)

I wanted to show it in code and just explain it, but I thought the best presentation could be a UML and explaination+code of the scenarios (it’s very simple to understand). Assume main flow (so simple UML):

Actually, that is about two bugs, but I abstracted that to have a general  better cleaning code.

1)Consider that in a new order (that is,update_multisig_params), we have common addresses between proposers (it's a general scenario, because the main problem is in the next part). At the first stage of executing this order, we should clean the common addresses from proposers, because as we know, a signer is a proposer with more features.

2)and a potential bug here (it's a multi-owner wallet! not a single owner!) is that we have a number of proposers and signers, we should check something when we have empty proposers:

1) (these orders -update_multisig_params- can be a diverting order , for example create common addresses in proposers, or even human errors , however it can help to prevent extra fee payments later ...) - (It's something like preventing sending limited message characters from Telegram on both sides, client and server , but needed to prevent later costs)

2) After that, we should consider this high priority point: the threshold should be more than one! because it seems multiowners are going through a critical state! and one signer can send a new order after that and take all funds or do anything because the signer(just a single signer in all signers) can send a new order and approve it immediately. Humans can not always prevent errors, so we can ignore any error, but we want to make the wallet so safe for each user (whether it be a developer user or a normal person).

where will be midified to solve the problem?

We should modify the multisig contract. (Totally, this problem can be solved in multiple ways, and it depends on the business plan.) The solution codes should be placed in the block code(the bugs is about that block, the main part that making new limitation and accessibility), the current code: https://github.com/ton-blockchain/multisig-contract-v2/blob/107ee13aa4cbabdc9ff0684b738dcd272c4211bc/contracts/multisig.func#L37-L49

and the modified code:

            } 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);

                int remainProposersNum = cleanCommonProposers(proposers, signers); ;; 1) cleaning the common addresses from proposers.
                throw_unless(error::invalid_new_order,(0 == remainProposersNum) & (min(max(threshold , 1),signers_num))); ;; or we can send a new order that can be double check by the signers , to be sure about that job! or change the threshold instead of throwing, that is not good job , but any solution that is ok for this problem ...

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

                action.end_parse();
            }

why we do that?

Every users cant understand how much it's critical if new order from another signers contains the common addresses(or bad idea behind their new order that contains the common addresses.)

Sorry for the weak English explanation. Thanks, regards.

tolya-yanot commented 6 months ago

threshold = 1 or proposers count = 0 are valid use cases

SC-One commented 6 months ago

ok (even withtogether), Thank you , how about that situation they have common addresses in new configs? it will pay more fees if they have common addresses.

tolya-yanot commented 6 months ago

Having common signers and proposers doesn't break the contract.

SC-One commented 6 months ago

Having common signers and proposers doesn't break the contract.

but checking that prevent more fee payments later! (in most cases.)