Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Atomically changing the owners of a unanimous multisig #4

Open
ruuda opened this issue May 3, 2021 · 5 comments
Open

Atomically changing the owners of a unanimous multisig #4

ruuda opened this issue May 3, 2021 · 5 comments

Comments

@ruuda
Copy link
Contributor

ruuda commented May 3, 2021

Suppose you create a multisig where the threshold is equal to the number of owners, and you want to add an owner (and bump the threshold by one).

At this point, I don’t think there is a way to do this atomically, is there? Because a transaction only holds one Instruction, you need to issue two calls, one for set_owners and one for change_threshold. If you bump the threshold first it can never be satisfied any more, so the new owner must be added before changing the threshold, which means there is a window of time in between where a non-unanimous subset of owners can execute a transaction. This set of owners can be different from the initial owners: if the new owner approves, then the transaction can be executed without consent from one of the initial owners.

This problem is not limited to the case where the number of owners is equal to the threshold, but it’s most apparent there.

One solution would be to merge set_owners and change_threshold into a single function. Another solution would be to allow multiple instructions in a single transaction.

@armaniferrante
Copy link
Member

armaniferrante commented May 3, 2021

Great point. We can probably just add an additional method for performing both the set_owners and change_threshold operation together. What do you think?

Also, if the runtime maintains the PDA as a signer in the entire CPI call hierarchy, then the behavior you describe can be achieved by calling a program that subsequently calls the other two methods (though I'd rather just bake this into the multisig itself, as it seems fundamental).

Edit. Confirmed the runtime should maintain the PDA signer across the CPI call hierarchy. (Though I haven't tested this).

@armaniferrante
Copy link
Member

armaniferrante commented May 3, 2021

@ruuda take a look at #5. It should address this use case. If not let me know.

@armaniferrante
Copy link
Member

armaniferrante commented May 3, 2021

Also, I think I'd prefer to keep this multisig as simple as possible, so I'd like to avoid allowing multiple instructions in the transaction data structure. If one needs multiple instructions, one can do it by calling an intermediate program via CPI, which then performs the multiple instructions.

@ruuda
Copy link
Contributor Author

ruuda commented May 4, 2021

then the behavior you describe can be achieved by calling a program that subsequently calls the other two methods

But both of them need to be approved first before you could do this, right? And those approvals are not going to be in a single transaction, so once one of them reaches the threshold, the first transaction can already be executed.

We can probably just add an additional method for performing both the set_owners and change_threshold operation together. What do you think?

Yes, that sounds good. If backwards compatibility is not an issue for you, I would even remove the separate setters to keep things simple.

@armaniferrante
Copy link
Member

Let's leave the separate ones there for now. Removing them would require some (quite minor) UI updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants