Manipulating Governance: Pre-Proposal Staking Adjustments to Sway Quorum Requirements

[c4-bot-4]

Lines of code

https://github.com/othernet-global/salty-io/blob/c46069644739885fa36e84e27e1dd6362b854663/src/dao/Proposals.sol#L410

Vulnerability details

C4 issue

https://github.com/code-423n4/2024-01-salty-findings/issues/716

In the issue described in link above, a vulnerability in the SALT staking system could allow stakers to manipulate the voting process by affecting the required quorum. When a proposal is up for vote, the required quorum for its approval is determined as a percentage of the total SALT staked. However, if a staker unstakes their SALT after voting, this action reduces the total staked amount, thereby lowering the required quorum without affecting the votes already cast.

Mitigation

https://github.com/othernet-global/salty-io/commit/c46069644739885fa36e84e27e1dd6362b854663

The mitigation effectively dealt with this issue by enabling Ballots to keep track of their own requiredQuorum at the time they were created, essentially locking in the required quorum on ballot creation and preventing post proposal manipulation.

New issue

However the introduction of this mitigation introduced a new attack vector as it allows for premeditated/pre-proposal manipulation of the required quorum by large or colluding stakers.

By adjusting their staked SALT amounts before a proposal is created via unstaking to decrease the total staked SALT, they can artificially lower the required quorum for that proposal. They can then cancel their unstaking to return their voting power to what is was and vote for in support of their proposal. This means they have effectively inflated their voting power without any financial penalty.

This strategy exploits the fixed quorum requirement to influence proposal outcomes, enabling large or colluding stakers to sway the governance process in their favor without directly engaging with the voting mechanism post-proposal creation.

In essence its also related to the original issue which the mitigation itself tried to fix, which is that the colluder's decrease in required quorum could allow borderline proposals to pass that normally couldn't meet quorum. However the mechanism of the attack is completely different(post vs pre proposal) and is only possible after the application of the mitigation.

Impact

The impact of this issue on governance and voting within a staking protocol can compromise the fairness and integrity of the decision-making process. By enabling large or colluding stakers to manipulate the required quorum in their favour for proposals through strategic unstaking actions, the vulnerability skews the democratic foundation of the governance model.

Proof Of Concept

  function testCanManipulateRequiredQuorum() public {
// Initial setup: Both Bob and Alice stake their amounts
uint256 bobInitialStake = 5000000 ether;
vm.startPrank(bob);
staking.stakeSALT(bobInitialStake);
vm.stopPrank();

vm.startPrank(alice);
staking.stakeSALT(1110111 ether); // Alice's stake
vm.stopPrank();

// Alice makes her proposal
vm.startPrank(alice);
uint256 aliceRequiredQuorum = proposals.requiredQuorumForBallotType(BallotType.PARAMETER);
vm.stopPrank();

// Bob unstakes a portion of his stake to reduce the total staked amount before making his proposal
vm.startPrank(bob);
uint256 unstakeid = staking.unstake(2500000 ether, 52); // Bob unstakes a portion before making his proposal
uint256 bobStakedAmount = staking.userShareForPool(bob, PoolUtils.STAKED_SALT);
uint256 bobRequiredQuorumBefore = proposals.requiredQuorumForBallotType(BallotType.PARAMETER);
staking.cancelUnstake(unstakeid);
uint256 bobStakedAmountAfter = staking.userShareForPool(bob, PoolUtils.STAKED_SALT);
// bob lowered his required quorum and returned his full voting power
assertLt(bobStakedAmount, bobStakedAmountAfter, "Bob's voting power should return back to what it was before the unstake");
vm.stopPrank();

//Check if Bob's proposal required quorum is indeed lower due to his earlier unstaking which reduced the total staked amount
// and hence the required quorum
assertLt(bobRequiredQuorumBefore, aliceRequiredQuorum, "Bob's proposal should have a lower required quorum due to his unstaking strategy");
vm.stopPrank();

}

The POC highlights how one actor can strategically reduce the required quorum for their proposal. It also shows that after lowering their required quorum, they can immediately cancel their unstaking, and get back their full voting power.

Tools used

Manual Review

Recommendation

    function cancelUnstake( uint256 unstakeID ) external nonReentrant
    {
    Unstake storage u = _unstakesByID[unstakeID];

    require( u.status == UnstakeState.PENDING, "Only PENDING unstakes can be cancelled" );
    require( block.timestamp < u.completionTime, "Unstakes that have already completed cannot be cancelled" );
          //@audit add cooldown period requirement here
    require( block.timestamp > u.coolDown, "Unstake can only happen after cooldown period has ended" );
    require( msg.sender == u.wallet, "Sender is not the original staker" );

    // Update the user's share of the rewards for staked SALT
    _increaseUserShare( msg.sender, PoolUtils.STAKED_SALT, u.unstakedXSALT, false );

    u.status = UnstakeState.CANCELLED;
    emit UnstakeCancelled(msg.sender, unstakeID);
    }

One potential solution that can be seen above is to use a cooldown period when attempting to cancel an unstake operation, whereby if users unstake their positions, they can only cancel their unstaking after a period of time has elapsed, long enough to allow active proposals(BallotMaximumDuration) to run their course before they're able to vote again using their unstaked->restaked tokens.

The cooldown period will essentially negate the benefits derived by reducing the required quorum, because manipulators will not be able to leverage their unstaked votes to use in the target proposal.

However note that this solution will need some thinking through and to be implemented carefully and in lieu of existing business logic, as it might interfere with the completionTime logic. For instance, If the completion time is less than the cooldown period, then a well intentioned user might be unable to cancel an unstake operation, once it has been initiated, even though the cooldown period has expired.

Assessed type

Governance

[Picodes]

Considering that there is a lower bound on the quorum that is based on the Salt total supply, it seems fair to consider that this is of Low severity and that eventual adjustments before the proposal is created have minimal impact because:

• voters have time to react
• the quorum can be decreased up to the lower bound so there still is a minimum amount of participation required

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[t0x1cC0de]

Considering that there is a lower bound on the quorum that is based on the Salt total supply, it seems fair to consider that this is of Low severity and that eventual adjustments before the proposal is created have minimal impact because:

• voters have time to react
• the quorum can be decreased up to the lower bound so there still is a minimum amount of participation required

Thanks @Picodes for the inputs.
Commenting here as I saw the reference inside my report: #6. I think there's some confusion here, so would like to present a few arguments.
In both M-11 and the new issue, the voters DO NOT have time to react. Also, the second point about quorum's lower bound was applicable in M-11 as well, so I am not quite clear how that is a differentiator or a reason enough to downgrade in this case.
Here's the explanation about the first point -

• In M-11, Alice could unstake after ballot end-time is reached (pre-requisite being that quorum was not reached yet) --> cause quorum reduction --> call finalizeBallot() and win before anyone else could react.
• In the current new issue, Alice can stake with partial funds --> create proposal with smaller quorum --> stake remaining funds as ballot end-time is reached (once again just as in M-11, pre-requisite being that quorum was not reached yet) --> call finalizeBallot() and win.
  • The aforementioned point is already mentioned in my report under "Attack Vector 1" as :
    • She now stakes her remaining aliceStakedAmount_2 = 570_000. This can be staked immediately after proposal creation or after a wait time of 14 days.

Happy to stand corrected in case I've misunderstood anything.

[Picodes]

@t0x1cC0de Thanks for your comment. I do agree with your reasoning when reconsidering. I'll validate as Med considering that because votes aren't snapshotted you could still manipulate the quorum either by unstaking before creating the proposal either by staking partially.

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as primary issue

[t0x1cC0de]

@t0x1cC0de Thanks for your comment. I do agree with your reasoning when reconsidering. I'll validate as Med considering that because votes aren't snapshotted you could still manipulate the quorum either by unstaking before creating the proposal either by staking partially.

Thanks @Picodes. Am I mistaken in noticing that this report highlights only one attack vector while my #6 shows coded PoC of both? I ask because this one got selected for report.

[c4-judge]

Picodes marked issue #6 as primary and marked this issue as a duplicate of 6

[Picodes]

@t0x1cC0de Thanks for your comment. I do agree with your reasoning when reconsidering. I'll validate as Med considering that because votes aren't snapshotted you could still manipulate the quorum either by unstaking before creating the proposal either by staking partially.

Thanks @Picodes. Am I mistaken in noticing that this report highlights only one attack vector while my #6 shows coded PoC of both? I ask because this one got selected for report.

Fair point. I'll reconsider. Note that the attack vector described here is way more important than the other. But your mitigation is also cleaner in my opinion.