Conflicting voter actions can lead to denial of service

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0xcbadf67e720bf94ab12c72a82292e95fe9bc6aa33d89f551a004f8617698038f Severity: medium

Description:

Impact

Voter motions with sufficient threshold can be blocked permanently by other voters

Description

Two issues introduce this vulnerability: even though there is a timelock on each motion's individual execution, the voters can create many motions simultaneously with no restriction on time.

The second one is that if threshold is half or less than the total number of voters, it's possible for voters to create conflicting actions, and the second or last motion always takes precedence if the first action is reversible or cancellable, which should not be the case as voters can now block valid motions with sufficient threshold.

Solving the issue with enforced threshold (higher than half the voters) would reduce the flexibility as it may be desired by the orchestrator, however an optional parameter to only allow a certain motion frequency can fix this.

Note that for proposal with multiple transactions TransactionForwarder_v1 can be used.

Proof of Concept

For demonstration purposes we name:

• X voters: A & B
• Y voters: C & D.
  1. Voting contract is initialized with a threshold of 2 and 4 voters.
  2. X voters create a proposal via createMotion() to add a voter E via addVoter(E) and cast their votes for (A, B)
  3. Y voters immediately create another motion that cancels out just this action via removeVoter(E) and cast their votes for (C, D)
  4. Timelock for the first motion passes, and E gets added as voter via executeMotion().
  5. Timelock passes for the second motion immediately after and E gets removed.

There are many possibilities with this issue within the contract and with every external action that is cancellable.

Recommendation

Consider to introduce a variable createMotionFrequency on createMotion() that only allows the function to be called every x seconds (settable at initialization and a potentially setter function for voters). Note that it can also be turned off by setting to zero to disallow this functionality.

AUT_EXT_VotingRoles_v1.sol - createMotion() (with proposed recommendation)

+   uint public lastCreatedMotion;
+   uint public createMotionFrequency;

    function createMotion(address target, bytes calldata action)
        external
        onlyVoter
        returns (uint)
    {
+       if (block.timestamp < lastCreatedMotion + createMotionFrequency) {
+          revert Module__VotingRoleManager__InvalidFrequency();
+       }
        // Cache motion's id.
        uint motionId = motionCount;

        // Get pointer to motion.
        // Note that the motion instance is uninitialized.
        Motion storage motion_ = motions[motionId];

        // Initialize motion.
        motion_.target = target;
        motion_.action = action;

        motion_.startTimestamp = block.timestamp;
        motion_.endTimestamp = block.timestamp + voteDuration;
        motion_.requiredThreshold = threshold;

        emit MotionCreated(motionId);

        // Increase the motion count.
        unchecked {
            ++motionCount;
        }

+       lastCreatedMotion = block.timestamp;

        return motionId;
    }

[0xfuje]

Title should be: Conflicting voter motions can lead to denial of service

[PlamenTSV]

Per comment from the sponsor, conflicts inbetween the voters is OOS, since majorities will always be formed. The initial value of the threshold is set by the Orchestrator and he holds all risk accommodated by it.