Proposal can be removed after 30 days without owner's consent

[c4-bot-6]

Lines of code

https://github.com/othernet-global/salty-io/blob/main/src/dao/DAO.sol#L256

Vulnerability details

Summary & Impact

The mitigation for M-19 was meant to ensure to avoid trapping the proposers indefinitely if their proposal had still not met quorum after 30 days. Hence the function manuallyRemoveBallot() is introduced which can be called by anyone.

The fix incorrectly assumes that the owner will always want their proposal to be cancelled after 30 days. This is not true. If their proposal is quite close to reaching quorum, the owner may want to keep it alive for a few more days. However, a griefer or someone who has voted against the ballot can choose to delete the proposal.

This choice of deleting the ballot ought to remain in the hands of only the proposal owner.

Proof of Concept

• Total staked salt = 6,000,000 in the system right now.
• Alice stakes her 650,000 salt. The total salt staked is now 6,000,000 + 650,000 = 6,650,000
• Alice floats her proposal.
• Minimum quorum required is 10% of 6,650,000 = 665,000
• Alice votes yes but this is still less than minimum required quorum.
• 30 days pass. Alice can see the proposal is tantalizingly close to reaching quorum and has an overwhelming majority of yes votes. So she plans to wait for another couple of days.
• Bob does not want the proposal to go through. Since 30 days have passed, he calls manuallyRemoveBallot() to remove the proposal.
• Alice can do nothing to stop Bob.

Add the following tests inside src/dao/tests/DAO.t.sol and run via COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.ankr.com/eth_sepolia --mt test_30dayRemoval to see the test pass:

  function test_30dayRemoval() public {
      // ********************* SETUP ********************************
      deal(address(salt), address(DEPLOYER), 6_000_000 ether);
      vm.prank(DEPLOYER);
      staking.stakeSALT(6_000_000 ether);

      // Set up the parameters for the proposal
      uint256 proposalNum = 0; // Assuming an enumeration starting at 0 for parameter proposals
      uint256 ballotID = 1;

      // Alice stakes her SALT to get voting power
      uint256 aliceStakedAmount = 650_000 ether;
      deal(address(salt), address(alice), aliceStakedAmount);
      // ************************************************************

      vm.startPrank(alice);
      staking.stakeSALT(aliceStakedAmount);
      // Propose a parameter ballot
      proposals.proposeParameterBallot(proposalNum, "Increase max pools count");
      // Alice casts a vote, but not enough for quorum
      // minQuorum required = 10% of (6_000_000 + 650_000) ether = 665_000 ether
      proposals.castVote(ballotID, Vote.INCREASE);
      assertEq(proposals.votesCastForBallot(ballotID, Vote.INCREASE), aliceStakedAmount);
      vm.stopPrank();

      skip(30 days);
      assertEq(proposals.ballotForID(ballotID).ballotIsLive, true, "Ballot should have been Live");

      // Bob calls `manuallyRemoveBallot()` to remove the proposal
      vm.prank(bob);
      dao.manuallyRemoveBallot(ballotID);
      assertEq(proposals.ballotForID(ballotID).ballotIsLive, false, "Ballot should have been removed");
  }

Recommended Mitigation Steps

Inside manuallyRemoveBallot(), ensure that msg.sender is the owner of the proposal.

Conclusion

New attack vector created due to the fix.

Assessed type

Access Control

[Picodes]

Downgrading to Low as there is no security issue here for me. If the proposal hasn't reached quorum within the voting period it can be cancelled, which seems fair.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[t0x1cC0de]

Added a comment regarding this in #94