Users can make last-minute votes and completely flip the results of a ballot

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L276-L290

Vulnerability details

Impact

The DAO is in control of very powerful actions, like changing price feeds used for borrowing and liquidations, changing the access manager, adding whitelisted tokens, and sending SALT from the DAO to any user, among other actions.

Users with enough staking (and therefore voting power) can vote on the very last possible block, flipping a ballot decision.

They can even signal the opposite vote during the whole course of the voting, and change it on the last minute.

Votes on the last minute have the same voting power as the ones casted before.

An adversary can take advantage of this. For example, they can propose malicious ballots with an account, and have another one with more voting power. The one with the most voting power can signal an "against" vote for the whole ballot duration to persuade other voters that the proposal will be rejected, and flip the vote in the last minute if they see they can win.

It is important to note that there is no penalization for creating malicious ballots over and over, and an adversary only needs to win one ballot to make some damage to the DAO and the protocol.

Note: This Impact fits into the Attack Ideas: "Any issue that would prevent the DAO from functioning correctly."

Vulnerability Details

When casting a vote, the userVotingPower represents 100% of the user share for the staked SALT pool, with no cap, nor affected by the time the vote was cast:

👉  uint256 userVotingPower = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
    require( userVotingPower > 0, "Staked SALT required to vote" );

    // Remove any previous votes made by the user on the ballot
    UserVote memory lastVote = _lastUserVoteForBallot[ballotID][msg.sender];

    // Undo the last vote?
    if ( lastVote.votingPower > 0 )
        _votesCastForBallot[ballotID][lastVote.vote] -= lastVote.votingPower;

    // Update the votes cast for the ballot with the user's current voting power
👉  _votesCastForBallot[ballotID][vote] += userVotingPower;

    // Remember how the user voted in case they change their vote later
    _lastUserVoteForBallot[ballotID][msg.sender] = UserVote( vote, userVotingPower );

Proposals.sol#L276-L290

It can also be seen that a user can first vote for an option, and switch to the opposite one on the last minute, without any penalization.

Proof of Concept

1. Add this test to src/dao/tests/Proposals.t.sol
2. Run COVERAGE="no" NETWORK="sep" forge test -vv --rpc-url https://1rpc.io/sepolia --mt "testSameVotingPowerAtEndOfBallot"

function testSameVotingPowerAtEndOfBallot() public {
    // Bob & Alice are accounts controlled by an adversary

    // Transfer some SALT to be able to create a proposal with one account
    vm.startPrank(DEPLOYER);
    salt.transfer(bob, 1000 ether);
    vm.stopPrank();

    // Bob creates a malicious proposal
    vm.startPrank(bob);
    staking.stakeSALT(1000 ether);
    address maliciousAddress = makeAddr("maliciousAddress");
    uint256 ballotID = proposals.proposeSetContractAddress("priceFeed1_confirm", maliciousAddress, "description");
    vm.stopPrank();

    // Alice first signals her vote "against" it
    vm.startPrank(alice);
    staking.stakeSALT(1000000 ether);
    proposals.castVote(ballotID, Vote.NO);
    vm.stopPrank();

    // Assert that "NO" is winning
    assertEq(proposals.votesCastForBallot(ballotID, Vote.NO), 1000000 ether);
    assertEq(proposals.votesCastForBallot(ballotID, Vote.YES), 0);

    // Warp time to the end of the voting phase
    // Other votes might not even vote given that "NO" is winning by a big amount
    vm.warp(block.timestamp + daoConfig.ballotMinimumDuration() + 1);

    // Alice flips her vote at the end of the voting phase as she sees that she'll win it
    vm.startPrank(alice);
    proposals.castVote(ballotID, Vote.YES);
    vm.stopPrank();

    // Assert she still has the same voting power, but flipped her decision
    assertEq(proposals.votesCastForBallot(ballotID, Vote.NO), 0);
    assertEq(proposals.votesCastForBallot(ballotID, Vote.YES), 1000000 ether);

    // Now she can finalize the ballot and change the price feed, for example here
}

Recommended Mitigation Steps

There are multiple ways to mitigate this. One easy way could be to linearly or exponentially decrease the voting power on the last lap of the voting phase. Other more sophisticated options are exposed on the Lido Proposal for "last minute vote mitigation", or the Time-weighted voting by Curve.

Assessed type

Governance

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

There are automatic confirmation votes associated with the setContract and websiteUpdate proposals. When those proposals succeed a mandatory confirmation vote is started to prevent last minute surprise votes.

[Picodes]

I do agree with the report but do not see how it fulfills the definition of Medium severity under C4's rules. To me, it's a design choice that could lead to vote results switching unexpectedly.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)