Multisig Permissions Extension

[area]

This PR implements a 'multisig permissions' extension. The underlying intention here is to make it so that powerful permissions (e.g. root) don't have to be under the control of a single EOA. So an action that the multisig should take can be suggested, and then if enough people (with the 'right' permissions) sign off on it, then it can be executed.

Specification

Should mirror the colony-level ('core' being the nomenclature I've adopted for those) permissions exactly - so being able to award permissions in specific domains, as well as being able to leverage permissions in child domains of the domains where the permissions have been explicitly awarded.

In terms of the 'threshold' for execution, the ideal functionality was some fraction of people with the relevant permissions. However, I couldn't establish a way to realistically do this, ~~and so went with an 'acceptable' threshold for 'votes' on an action before it could be executed. If a reviewer comes up with a way to implement fraction-based rather than absolute-based approvals, I'd be interested to hear it.~~

Permissions work as such:

• If a fixed threshold for the relevant domain is set, that number is used
• Otherwise, if a fixed global threshold is set, that number is used
• Otherwise, n/2 + 1 approvals are required, where n is the number of people who explicitly have the relevant permission in the domain in question. People with the permission inherited can approve, but do not count towards the calculation of the threshold.

Less prose-like requirements:

• Removal of a permission doesn't remove approvals already made, and you can remove your approvals even if the permission that originally allowed you to make an approval is removed.
• Anyone can execute an action if it has enough approvals.
• Overapprovals are allowed
• To suggest an action, you require ~the permissions~ at least one of the permissions that would be needed to approve it, and you automatically approve it on creation.
• Multiple transactions can be suggested in a single action (either natively, or via multicall), but they must all require the same permissions.
• If an action fails on execution, the transaction reverts if it's been less than a week since it was eligible for execution. Otherwise, the action fails, though the transaction succeeds overall, and the motion will not be able to be retried.
• Core permissions can award multisig permissions they could award core versions of, and vice-versa (if the threshold is reached when going via multisig)

Implementation notes

There is some arguably surprising behaviour around what happens if the threshold is changed and motions have already had approvals made, but I've tried to make it as straightforward as possible; if an approval/unapproval causes a motion to cross the threshold, the timestamp is updated. If the timestamp is 0, but we're able to execute, the call of execute will update the timestamp to now, and do nothing else. A second call to execute will actually execute the motion. I'm very open to suggestions here, as I'm not totally happy with where I've landed.

Where multiple permissions are required for a function call (currently the case for OneTxPayment, and nothing else), each permission has a separate threshold, and both must be met for execution to be allowed.

[arrenv]

If an action fails on execution, the transaction reverts if it's been less than a week since it was eligible for execution. Otherwise, the transaction succeeds and the motion will not be able to be retried.

Should this be "Otherwise, the transaction fails and the motion will not be able to be retried."?

[arrenv]

There is some arguably surprising behaviour around what happens if the threshold is changed and motions have already had approvals made, but I've tried to make it as straightforward as possible; if an approval/unapproval causes a motion to cross the threshold, the timestamp is updated. If the timestamp is 0, but we're able to execute, the call of execute will update the timestamp to now, and do nothing else. A second call to execute will actually execute the motion. I'm very open to suggestions here, as I'm not totally happy with where I've landed.

What I understand from this is that we have these scenarios:

• Motion HAS passed the threshold and CAN be finalized, a new EOA is awarded the same permissions and now the threshold changes and now the motion CANNOT be finalized until another approval is given.
• Motion HAS NOT passed the threshold, an EOA has their permissions revoked and now the threshold changes and now the motion CAN be finalized.

However, you have a timestamp to account for this, but that requires an additional transaction each time? Can they be bundled?

Am I right in this understanding?

[area]

If an action fails on execution, the transaction reverts if it's been less than a week since it was eligible for execution. Otherwise, the transaction succeeds and the motion will not be able to be retried.

Should this be "Otherwise, the transaction fails and the motion will not be able to be retried."?

It should be "Otherwise, the action fails, though the transaction succeeds overall, and the motion will not be able to be retried".

[area]

What I understand from this is that we have these scenarios:

* Motion HAS passed the threshold and CAN be finalized, a new EOA is awarded the same permissions and now the threshold changes and now the motion CANNOT be finalized until another approval is given.

* Motion HAS NOT passed the threshold, an EOA has their permissions revoked and now the threshold changes and now the motion CAN be finalized.

However, you have a timestamp to account for this, but that requires an additional transaction each time? Can they be bundled?

Am I right in this understanding?

First off, the number of addresses with a given permission is independent of the threshold, which is just a fixed number. If that many people or more have approved something, it can be called.

In the first scenario, let's say 2 people have approved a motion (which was sufficient for execution, and set the timestamp) and the threshold is now 3. Attempts to execute will fail (as we do not meet the threshold). If a third approves, the timestamp will be updated, and the motion will have a week as expected before failed executions are allowed. No additional transactions are required.

In the second scenario, let's say 2 people had approved a motion, with a threshold of 3. The threshold is now moved to 2, and so the motion can be executed, but the timestamp hasn't been set. The first call of execute will set the timestamp and do nothing else. A second call of execute will actually execute the motion (or try to, if it fails). These can be bundled if the motion is going to execute successfully, but if the motion fails and the transaction reverts, if you have bundled them then the timestamp will also not be set, and so the 'clock' for allowing a failing execution will not have started.

I'll also note that in normal activity (i.e. the threshold not being changed), only one execute call is ever required.

[arrenv]

First off, the number of addresses with a given permission is independent of the threshold, which is just a fixed number. If that many people or more have approved something, it can be called.

This is on me, as we spoke about this but I misunderstood. I had understood our two options were a configurable fixed threshold which was difficult and the other was a broad approach using the same formula as exiting recovery numRequired = totalAuthorized / 2 + 1. Although, I distinctly remember you saying it needed to be fixed and I seemed to have perceived that as meaning the opposite. So, now I have questions:

• How is this set? Is it per Colony or Colony Network wide?
• Is it configurable on extension enabling?
• Is it changeable after it has been set?
• Is it configurable per permission type and per team?
• If it is set to a value higher then the number of permission holders, does that mean Motions can never be approved?

In the second scenario, let's say 2 people had approved a motion, with a threshold of 3. The threshold is now moved to 2, and so the motion can be executed, but the timestamp hasn't been set. The first call of execute will set the timestamp and do nothing else. A second call of execute will actually execute the motion (or try to, if it fails). These can be bundled if the motion is going to execute successfully, but if the motion fails and the transaction reverts, if you have bundled them then the timestamp will also not be set, and so the 'clock' for allowing a failing execution will not have started.

So, basically by bundling them you can get around the 1 week rule, in a sense. So, they need to be two transactions so that the rule stays in place.

EDIT:

These were discussed on a call.

How is this set? Is it per Colony or Colony Network wide?

• This is set per Colony.

Is it configurable on extension enabling?

• You configure it on initialization.

Is it changeable after it has been set?

• You can change it after initialization using Root permissions.

Is it configurable per permission type and per team?

• Not configurable per type or per team

If it is set to a value higher then the number of permission holders, does that mean Motions can never be approved?

• This is correct. However, if this is an issue in child domain, anyone with parent team permissions would also be permission holders.

UPDATES: Based on this, it has been decided to modify to allow the following options:

• Single fixed Colony wide threshold, which is changeable by the root permission.
• Relative value dependent on the number of explicit permission holders of the team, not inherited
• A fixed threshold configurable per team.

[area]

Relative value dependent on the number of explicit permission holders of the team, not inherited

Just noting this explicitly/precisely for posterity

[area]

I'm not sure I've ever had someone say 'there are too many comments'. They certainly originated from when I was thinking through the implementation, but what's your reasoning here for removing them? Do you not see them as helpful?

[kronosapiens]

@area ultimately the comments read to me like they were written to describe the logical flow of the contract to help you implement, rather than comments designed to add useful clarifying information. Now that the implementation is done, I think the code can stand on its own. Redundant comments create additional complexity when parsing a contract, and so I would remove them.

Example:

          if (_approved) {
            // They are now approving it

[area]

Need the ability to 'reject' a transaction, not just not-approve it.

[arrenv]

Need the ability to 'reject' a transaction, not just not-approve it.

The spec has been updated here - https://www.notion.so/colony/Multi-sig-Permissions-6aaf0b95309f4754a243d54e8d9c4189?pvs=4

Additions:

• User story

  • As a User, I want to be able to cancel a proposed action as the owner, and collectively if not the owner. This prevents the action from being able to be indefinitely open if not initially approved.

• Acceptance criteria:

  • Cancelling multi-sig actions
    • It should be possible for the owner of the multi-sig action to be able to cancel their own multi-sig action.
    • It should be possible for other permission holders to collectively cancel a multi-sig action.

Design for the cancellation/rejection process are here - https://www.figma.com/file/5V8pr7iMwXsT9L3VAZsmUt/Colony-V5?type=design&node-id=6177%3A354823&mode=design&t=hoAwbRfZm8kN9beP-1

[kronosapiens]

@area @arrenv can this be implemented through some type of expiration window? e.g. after 7 days if the motion is not approved, it is automatically cancelled?

[arrenv]

@area @arrenv can this be implemented through some type of expiration window? e.g. after 7 days if the motion is not approved, it is automatically cancelled?

As the only alternative option, or as an additional option?

Would it work in a way where if not enough people sign in the time window, then it auto cancels?

[kronosapiens]

I would suggest doing it as the only option, where if it doesn't pass in the window it automatically fails. I suspect that will be much easier to implement (and use) than a parallel downvote system, but I'll let @area weigh in

[arrenv]

I would suggest doing it as the only option, where if it doesn't pass in the window it automatically fails. I suspect that will be much easier to implement (and use) than a parallel downvote system, but I'll let @area weigh in

What about the owner being able to cancel their own created action, would that still work with this approach? As it is something Motions is getting as well.

[kronosapiens]

@arrenv that should be fine, curious what @area thinks

[area]

Personally, I don't think that the creator of a multisig action should have any special permissions compared to anyone else with the equivalent permissions on a multisig. That is mostly a personal preference rather than anything technical though, and is indeed easy to do.

I agree a timeout is easier to implement, though it doesn't solve the primary issue around why rejections are required, it only ameliorates it. If something is proposed, partially approved, forgotten about, and then the thresholds change, a motion might silently become executable that does something that is no longer (or was never, given it was never sufficiently approved) wanted. Limiting that to seven days makes it less likely, but does not remove the issue.

I have put support for rejections that I believe meets the supplied additional spec in another branch while we continue this discussion.

[kronosapiens]

@area I hear that, but I'm also noting that the addition of rejection functionality adds many hundreds of lines to the contract+tests, whereas a timeout-based rejection would probably require a dozen lines or less. From a perspective of code complexity and risk, I would argue for a timeout-based approach.

The scenario of a motion being partially approved, "forgotten about" while simultaneously having thresholds lowered to the point where it can be maliciously executed is not a scenario which deeply troubles me, especially if the timeout is set to something judicious, on the order of ~3 days.

Thinking more, I would even be in favor of resetting the approval count entirely whenever the threshold changes, which I expect would allow us to remove a lot of complicated existing logic and sidestep these scenarios entirely.

[area]

Rebased on #1237 on request of Chris/Kerry, and converted to draft to prevent accidental merging.

[area]

The spec is now much more comprehensive and we should make sure we still meet it. Of note:

• Giving permissions requires Admin level or higher tiered permissions (Architectur or Root custom and contract level permissions). This should work with both the core permissions and the multi-sig permissions. With multi-sig still requiring consensus via the multi-sig approval mechanism.

[area]

Should auto-cancel if threshold is met

EDIT: Now included

[area]

Should have the same 'default address' functionality with 0x00000...0000 as motions.

EDIT: Now included.

[area]

Comment to please Chewie, because I haven't taken in to account adding/removing tags, which I probably should :sweat_smile:

[area]

Just a reminder to be careful when doing refactoring or copying and pasting (or maybe using Copilot?) - I've found spots where tests you've added or edited have either been meaningfully changed or don't test what they claim

I'm a little confused by what coverage is telling me with the current state of the PR. There is a branch uncovered that I would have otherwise thought should be being hit. Do you have any idea what's going on there?

[kronosapiens]

@area do you mean the test here? At first glance I agree with you... it seems strange.

[area]

I think I've decided that this is an issue with solidity-coverage. Applying this diff:

diff --git a/contracts/extensions/MultisigPermissions.sol b/contracts/extensions/MultisigPermissions.sol
index 1b5350a3..4837281d 100644
--- a/contracts/extensions/MultisigPermissions.sol
+++ b/contracts/extensions/MultisigPermissions.sol
@@ -311,13 +311,13 @@ contract MultisigPermissions is ColonyExtensionMeta, ColonyDataTypes, GetActionS
     // Allow this function to be called if the caller:
     require(
       // Has core root permissions OR
-      colony.hasUserRole(msgSender(), 1, ColonyDataTypes.ColonyRole.Root) ||
+      !(!colony.hasUserRole(msgSender(), 1, ColonyDataTypes.ColonyRole.Root) &&
         // Has core architecture, if we're using that permission in a child domain
-        (colony.hasUserRole(
+        !(colony.hasUserRole(
           msgSender(),
           _permissionDomainId,
           ColonyDataTypes.ColonyRole.Architecture
-        ) && (_permissionDomainId != _domainId)),
+        ) && (_permissionDomainId != _domainId))),
       "multisig-caller-not-correct-permissions"
     );
 

with no other changes results in that branch being marked as covered (and covered both ways, as expected). I believe that !(!x && !(y&&z)) is logically equivalent to x || (y && z) and I have applied that transformation correctly to the code. If you agree, and it behaves the same for you if you run with the above diff (git apply file.diff) then I'm happy for this to be merged.

EDIT: Actually, it's that solidity-coverage doesn't perceive a branch there with the second construction, which is making me a little hesitant about my conclusion of 'everything is fine'.

[area]

Got it. This one was a subtle one.

"Rewrite entire test suite in Typescript" moves up a rung on my priorities 😅

[kronosapiens]

FWIW codebase migrations to TS is the sort of thing I'd expect LLMs to be able to start handling soon.

https://about.grit.io/

[area]

I've done a bit of a larger refactor than I intended with getActionSummary. This stemmed from the fact that I was uncomfortable that NO_ACTION had special meaning in Multisig, when that shouldn't have been the case. By returning early again, I think we've been able to remove some if statements (as they shouldn't be able to be hit), and I also realised that the refactor to revert in the helper meant some requires could be removed from VotingReputation. I'd appreciate a careful review here - it's not too imposing, but a misplaced ! would really ruin our day :smile:

My second commit here is to fix an issue that Kerry spotted, which was that the architecture role could be used to create and vote on motions to award permissions in the domain where the architecture role was held (when it should only be allowed in subdomains of where the permission is held, mirroring the 'core' permission behaviour). That's a much cleaner set of changes!