ton-blockchain / multisig-contract-v2

Multiowner wallet
54 stars 19 forks source link

Probably it is possible to update_multisig_params, but order will not be invalidated #42

Closed SubFactorial closed 6 months ago

SubFactorial commented 6 months ago

If it is possible to update_multisig_params such way that change params one time to some other signers list and after that change params again and set the old signers list (probably, it is possible to set reference to old cell with signers dictionary somehow with low-level code) then order will not be invalidated, because signers list the same, although it should be invalidated.

And if it is possible to update_multisig_params such way that signers list is not changed (by giving the same reference to dictionary), then it is possible to change threshold value. In mutisig.func in line 166 it is checked only that hash of signers cell the same, but it is not checked that threshold might be changed.

nns2009 commented 6 months ago

I see you are new to FunC, but you made some good guesses in your issues. This one is the closest to be real.

probably, it is possible to set reference to old cell with signers

For your information, there is no "referential equality" in TON. Everything is compared "by value". In this case it's this part: signers_hash == signers.cell_hash() which compares hashes (which is equivalent to comparison "by value"). So...

change params one time to some other signers list and after that change params again and set the old signers list ... then order will not be invalidated

yes: re-setting the old list of signers will indeed not invalidate corresponding orders. But that seems intended as per description:

More strictly, Order is only valid when current signers are equal to signers at the time Order was created.

and I also don't see a problem with such behavior.

then it is possible to change threshold value

For a moment I thought this part is an actual flaw. But it in fact won't weaken security. In case of:

Mayowaking commented 6 months ago

GOOD great and superb

Mayowaking commented 6 months ago

GOOD great and superb

EmelyanenkoK commented 6 months ago

As Nns2009 wrote above, indeed, setting new signers set will invalidate old orders, but setting old signers set back will restore validity (where applicable). This behavior is considered as part of documented behavior of contract.

The same stands for changing threshold: if threshold is lowered on multisig, old order will be executed but only when it gets older (higher) number of approvals. If threshold is increased on multisig, old order can not be executed due to check on multisig (that is actually why we need to send approvals_num from order to multisig), at least until threshold is not lowered back.

SubFactorial commented 6 months ago

"old order will be executed but only when it gets older (higher) number of approvals" Probably, it will start executing when threshold is equal to approvals_num. But if consider long lasting orders, which might call execute_internal operation or create new orders. I think that in these cases threshold might be decreased (if other order lowered threshold and set the same signers list) and initial order or some tail of initial order (execute_internal) might be executed when approvals_num != threshold, because in condition is approvals_num >= threshold: In multisig.func line 166 throw_unless(error::singers_outdated, (signers_hash == signers.cell_hash()) & (approvals_num >= threshold));

SubFactorial commented 6 months ago

"This behavior is considered as part of documented behavior of contract."

It is more safely to implement more strict condition: everything or nothing. That if some option is changed then it will invalidate orders which were approved with this options. It will give simpler reasoning rules. Otherwise it would be necessary to imagine tricky situations like:

tolya-yanot commented 6 months ago

@nns2009 and @EmelyanenkoK are right. No problem here